Merge lp:~marcustomlinson/v8-cpp/test-gc into lp:v8-cpp

Proposed by Marcus Tomlinson on 2015-08-31
Status: Merged
Approved by: Marcus Tomlinson on 2015-09-09
Approved revision: 34
Merged at revision: 28
Proposed branch: lp:~marcustomlinson/v8-cpp/test-gc
Merge into: lp:v8-cpp
Diff against target: 533 lines (+222/-88)
7 files modified
src/internal/class.h (+102/-39)
src/internal/convert.h (+35/-27)
src/run.h (+5/-0)
tests/objects/module.cpp (+2/-0)
tests/objects/test.cpp (+59/-16)
tests/objects/test.h (+17/-5)
valgrind.supp (+2/-1)
To merge this branch: bzr merge lp:~marcustomlinson/v8-cpp/test-gc
Reviewer Review Type Date Requested Status
Alexandre Abreu (community) 2015-08-31 Approve on 2015-09-08
Review via email: mp+269594@code.launchpad.net

Commit message

Added support for shared_ptr and unique_ptr

To post a comment you must log in.
Alexandre Abreu (abreu-alexandre) wrote :

small comment inline

Marcus Tomlinson (marcustomlinson) wrote :

Replied to inline comments.

Alexandre Abreu (abreu-alexandre) wrote :

replied

Marcus Tomlinson (marcustomlinson) wrote :

> replied

Ok I see what you're saying. I'll try specifically set the internal field counts to 2 and 3 for non-shared_ptr and shared_ptr objects respectively.

Marcus Tomlinson (marcustomlinson) wrote :

Hmmm, so I've untangled my thoughts and realised again why it has to remain implemented this way.

The internal field count is set when creating the class template. This template is what we use to mould instances of the class as they are created. We only manage exported class types, even in the case of smart pointers, we are actually instantiating the underlying class type template and appending the extra smart pointer reference as the 3rd internal field.

Seeing that we need our class templates to support both normal instances as well as smart pointer instances, we have to make that 3rd internal field available in all of our templates as either variant may be constructed from them.

Hope I'm making sense?

Alexandre Abreu (abreu-alexandre) wrote :

> Hmmm, so I've untangled my thoughts and realised again why it has to remain
> implemented this way.
>
> The internal field count is set when creating the class template. This
> template is what we use to mould instances of the class as they are created.
> We only manage exported class types, even in the case of smart pointers, we
> are actually instantiating the underlying class type template and appending
> the extra smart pointer reference as the 3rd internal field.
>
> Seeing that we need our class templates to support both normal instances as
> well as smart pointer instances, we have to make that 3rd internal field
> available in all of our templates as either variant may be constructed from
> them.
>
> Hope I'm making sense?

totally,

mmh changing that would require quite a bit of "redoing", we might as well leave this as is, and update it if necessary,

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/internal/class.h'
2--- src/internal/class.h 2015-08-05 18:57:44 +0000
3+++ src/internal/class.h 2015-09-01 09:57:53 +0000
4@@ -22,6 +22,12 @@
5 #include <internal/function.h>
6 #include <internal/utility/object_factory.h>
7
8+#include <memory>
9+
10+template <typename T> struct IsSharedPointer : std::false_type {};
11+template <typename T> struct IsSharedPointer<std::shared_ptr<T>> : std::true_type { using type = T; };
12+template <typename T> struct IsSharedPointer<std::shared_ptr<T const>> : std::true_type { using type = T; };
13+
14 namespace v8cpp
15 {
16 namespace internal
17@@ -44,7 +50,7 @@
18 }
19
20 // Find Class instance by class type
21- size_t my_type = class_type_();
22+ size_t my_type = class_type();
23 Class* result;
24 if (my_type < instances->size())
25 {
26@@ -96,50 +102,111 @@
27 proto_template()->Inherit(base_class->class_template());
28 }
29
30- v8::Local<v8::Object> export_object(T* object)
31+ template <typename O>
32+ typename std::enable_if<IsSharedPointer<O>::value, v8::Local<v8::Object>>::type export_object(O* object, bool)
33 {
34 v8::EscapableHandleScope scope(isolate_);
35
36- v8::Local<v8::Object> v8_object = class_template()->GetFunction()->NewInstance();
37- v8_object->SetAlignedPointerInInternalField(0, object);
38- v8_object->SetAlignedPointerInInternalField(1, this);
39+ using raw_ptr_class = Class<typename IsSharedPointer<T>::type>;
40+ v8::Local<v8::Object> v8_object = raw_ptr_class::instance(isolate_).class_template()->GetFunction()->NewInstance();
41+ v8_object->SetAlignedPointerInInternalField(0, object->get());
42+ v8_object->SetAlignedPointerInInternalField(1, &raw_ptr_class::instance(isolate_));
43+ v8_object->SetAlignedPointerInInternalField(2, object);
44
45 MoveablePersistent<v8::Object> v8_object_p(isolate_, v8_object);
46 v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
47 {
48 v8::Isolate* isolate = data.GetIsolate();
49 T* object = data.GetParameter();
50- instance(isolate).destroy_object(object);
51+ raw_ptr_class::instance(isolate).remove_object<T>(isolate, object, &ObjectFactory<T>::delete_object);
52 });
53
54- ClassInfo::add_object(object, std::move(v8_object_p));
55-
56- return scope.Escape(v8_object);
57- }
58-
59- v8::Local<v8::Object> export_object(T const* object)
60- {
61- return export_object(const_cast<T*>(object));
62+ raw_ptr_class::instance(isolate_).add_object(object, std::move(v8_object_p));
63+
64+ return scope.Escape(v8_object);
65+ }
66+
67+ template <typename O>
68+ typename std::enable_if<!IsSharedPointer<O>::value, v8::Local<v8::Object>>::type export_object(O* object, bool gc)
69+ {
70+ v8::EscapableHandleScope scope(isolate_);
71+
72+ v8::Local<v8::Object> v8_object = class_template()->GetFunction()->NewInstance();
73+ v8_object->SetAlignedPointerInInternalField(0, object);
74+ v8_object->SetAlignedPointerInInternalField(1, this);
75+ v8_object->SetAlignedPointerInInternalField(2, nullptr);
76+
77+ MoveablePersistent<v8::Object> v8_object_p(isolate_, v8_object);
78+ if (gc)
79+ {
80+ v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
81+ {
82+ v8::Isolate* isolate = data.GetIsolate();
83+ T* object = data.GetParameter();
84+ instance(isolate).remove_object<T>(isolate, object, &ObjectFactory<T>::delete_object);
85+ });
86+ }
87+ else
88+ {
89+ v8_object_p.SetWeak(object, [](v8::WeakCallbackData<v8::Object, T> const& data)
90+ {
91+ v8::Isolate* isolate = data.GetIsolate();
92+ T* object = data.GetParameter();
93+ instance(isolate).remove_object<T>(isolate, object, nullptr);
94+ });
95+ }
96+
97+ add_object(object, std::move(v8_object_p));
98+
99+ return scope.Escape(v8_object);
100+ }
101+
102+ v8::Local<v8::Object> export_object(T const* object, bool gc)
103+ {
104+ return export_object(const_cast<T*>(object), gc);
105 }
106
107 v8::Local<v8::Object> export_object(v8::FunctionCallbackInfo<v8::Value> const& args)
108 {
109- return constructor_ ? export_object(constructor_(args)) :
110+ return constructor_ ? export_object(constructor_(args), true) :
111 throw std::runtime_error("exported class does not have a constructor specified");
112 }
113
114- T* import_object(v8::Local<v8::Value> value)
115- {
116- v8::HandleScope scope(isolate_);
117-
118- while (value->IsObject())
119- {
120- v8::Local<v8::Object> object = value->ToObject();
121- if (object->InternalFieldCount() == 2)
122- {
123- void* ptr = object->GetAlignedPointerFromInternalField(0);
124- ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
125- if (info && info->cast(ptr, class_type_()))
126+ template <typename O>
127+ typename std::enable_if<IsSharedPointer<O>::value, O*>::type import_object(v8::Local<v8::Value> value)
128+ {
129+ v8::HandleScope scope(isolate_);
130+
131+ while (value->IsObject())
132+ {
133+ v8::Local<v8::Object> object = value->ToObject();
134+ if (object->InternalFieldCount() == 3)
135+ {
136+ void* ptr = object->GetAlignedPointerFromInternalField(0);
137+ ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
138+ if (info && info->cast(ptr, Class<typename IsSharedPointer<T>::type>::instance(isolate_).class_type()))
139+ {
140+ return static_cast<T*>(object->GetAlignedPointerFromInternalField(2));
141+ }
142+ }
143+ value = object->GetPrototype();
144+ }
145+ return nullptr;
146+ }
147+
148+ template <typename O>
149+ typename std::enable_if<!IsSharedPointer<O>::value, O*>::type import_object(v8::Local<v8::Value> value)
150+ {
151+ v8::HandleScope scope(isolate_);
152+
153+ while (value->IsObject())
154+ {
155+ v8::Local<v8::Object> object = value->ToObject();
156+ if (object->InternalFieldCount() == 3)
157+ {
158+ void* ptr = object->GetAlignedPointerFromInternalField(0);
159+ ClassInfo* info = static_cast<ClassInfo*>(object->GetAlignedPointerFromInternalField(1));
160+ if (info && info->cast(ptr, class_type()))
161 {
162 return static_cast<T*>(ptr);
163 }
164@@ -149,11 +216,6 @@
165 return nullptr;
166 }
167
168- void destroy_object(T* object)
169- {
170- ClassInfo::remove_object(isolate_, object, &ObjectFactory<T>::delete_object);
171- }
172-
173 template <typename P>
174 static void get_member(v8::Local<v8::String>, v8::PropertyCallbackInfo<v8::Value> const& info)
175 {
176@@ -177,6 +239,12 @@
177 self.*property_ptr = from_v8<PropType>(isolate, value);
178 }
179
180+ static size_t class_type()
181+ {
182+ static size_t my_type = ClassInfo::register_class();
183+ return my_type;
184+ }
185+
186 private:
187 explicit Class(v8::Isolate* isolate, size_t type)
188 : ClassInfo(type)
189@@ -205,13 +273,8 @@
190
191 // Each JS instance has a built-in field holding a pointer to the C++ object
192 // We add a second field to hold a pointer to this Class instance
193- class_template->InstanceTemplate()->SetInternalFieldCount(2);
194- }
195-
196- static size_t class_type_()
197- {
198- static size_t my_type = ClassInfo::register_class();
199- return my_type;
200+ // And an optional third field to hold a smart pointer
201+ class_template->InstanceTemplate()->SetInternalFieldCount(3);
202 }
203
204 v8::Isolate* isolate_;
205
206=== modified file 'src/internal/convert.h'
207--- src/internal/convert.h 2015-08-05 18:57:44 +0000
208+++ src/internal/convert.h 2015-09-01 09:57:53 +0000
209@@ -21,6 +21,7 @@
210 #include <climits>
211 #include <vector>
212 #include <map>
213+#include <memory>
214
215 #include <v8.h>
216
217@@ -369,40 +370,38 @@
218
219 // Exported class converters
220 template <typename T>
221-struct IsExportedClass : std::is_class<T>
222-{
223-};
224-
225-template <typename T>
226-struct IsExportedClass<v8::Local<T>> : std::false_type
227-{
228-};
229-
230-template <typename T>
231-struct IsExportedClass<v8::Persistent<T>> : std::false_type
232-{
233-};
234+struct IsExportedClass : std::is_class<T> {};
235+
236+template <typename T>
237+struct IsExportedClass<v8::Local<T>> : std::false_type {};
238+
239+template <typename T>
240+struct IsExportedClass<v8::Persistent<T>> : std::false_type {};
241
242 template <typename Char, typename Traits, typename Alloc>
243-struct IsExportedClass<std::basic_string<Char, Traits, Alloc>> : std::false_type
244-{
245-};
246+struct IsExportedClass<std::basic_string<Char, Traits, Alloc>> : std::false_type {};
247
248 template <typename T, typename Alloc>
249-struct IsExportedClass<std::vector<T, Alloc>> : std::false_type
250-{
251-};
252+struct IsExportedClass<std::vector<T, Alloc>> : std::false_type {};
253
254 template <typename Key, typename Value, typename Less, typename Alloc>
255-struct IsExportedClass<std::map<Key, Value, Less, Alloc>> : std::false_type
256-{
257-};
258+struct IsExportedClass<std::map<Key, Value, Less, Alloc>> : std::false_type {};
259+
260+template <typename T>
261+struct IsUniquePointer : std::false_type {};
262+
263+template <typename T>
264+struct IsUniquePointer<std::unique_ptr<T>> : std::true_type { using type = T; };
265+
266+template <typename T>
267+struct IsUniquePointer<std::unique_ptr<T const>> : std::true_type { using type = T; };
268
269 template <typename T>
270 struct Convert<T, typename std::enable_if<IsExportedClass<T>::value>::type>
271 {
272 using FromType = T&;
273 using ToType = v8::Local<v8::Object>;
274+ using ClassType = typename std::remove_cv<T>::type;
275
276 static bool is_valid(v8::Isolate*, v8::Local<v8::Value> value)
277 {
278@@ -422,9 +421,18 @@
279 throw std::runtime_error("expected an exported object");
280 }
281
282- static ToType to_v8(v8::Isolate* isolate, T const& value)
283- {
284- return Convert<T*>::to_v8(isolate, new T(value));
285+ template <typename O>
286+ static typename std::enable_if<IsUniquePointer<O>::value, ToType>::type to_v8(v8::Isolate* isolate, O const& value)
287+ {
288+ // Convert unique pointers to shared pointers before exposing them to v8
289+ std::shared_ptr<typename IsUniquePointer<T>::type> s_ptr{std::move(const_cast<T&>(value))};
290+ return Convert<std::shared_ptr<typename IsUniquePointer<T>::type>>::to_v8(isolate, s_ptr);
291+ }
292+
293+ template <typename O>
294+ static typename std::enable_if<!IsUniquePointer<O>::value, ToType>::type to_v8(v8::Isolate* isolate, O const& value)
295+ {
296+ return Class<ClassType>::instance(isolate).export_object(new T(value), true);
297 }
298 };
299
300@@ -446,12 +454,12 @@
301 {
302 throw std::invalid_argument("expected an object");
303 }
304- return Class<ClassType>::instance(isolate).import_object(value);
305+ return Class<ClassType>::instance(isolate).template import_object<T>(value);
306 }
307
308 static ToType to_v8(v8::Isolate* isolate, T const* value)
309 {
310- return Class<ClassType>::instance(isolate).export_object(value);
311+ return Class<ClassType>::instance(isolate).export_object(value, false);
312 }
313 };
314
315
316=== modified file 'src/run.h'
317--- src/run.h 2015-08-05 18:57:44 +0000
318+++ src/run.h 2015-09-01 09:57:53 +0000
319@@ -32,6 +32,11 @@
320 // Create an isolate
321 std::shared_ptr<v8::Isolate> isolate(v8::Isolate::New(), [](v8::Isolate* isolate)
322 {
323+ // Force garbage collection before returning
324+ std::string const v8_flags = "--expose_gc";
325+ v8::V8::SetFlagsFromString(v8_flags.data(), (int)v8_flags.length());
326+ isolate->RequestGarbageCollectionForTesting(v8::Isolate::GarbageCollectionType::kFullGarbageCollection);
327+
328 // Clean up
329 using ClassInstances = std::vector<v8cpp::internal::Class<void>*>;
330 ClassInstances* instances = static_cast<ClassInstances*>(isolate->GetData(0));
331
332=== modified file 'tests/objects/module.cpp'
333--- tests/objects/module.cpp 2015-08-05 18:57:44 +0000
334+++ tests/objects/module.cpp 2015-09-01 09:57:53 +0000
335@@ -32,6 +32,8 @@
336 testclass
337 .set_constructor<int, int>()
338 .add_method("i", &TestClass::i)
339+ .add_method("embedded_class_sptr", &TestClass::embedded_class_sptr)
340+ .add_method("embedded_class_uptr", &TestClass::embedded_class_uptr)
341 .add_method("embedded_class_ptr", &TestClass::embedded_class_ptr)
342 .add_method("embedded_class_ref", &TestClass::embedded_class_ref)
343 .add_method("embedded_class_copy", &TestClass::embedded_class_copy)
344
345=== modified file 'tests/objects/test.cpp'
346--- tests/objects/test.cpp 2015-07-08 09:29:21 +0000
347+++ tests/objects/test.cpp 2015-09-01 09:57:53 +0000
348@@ -54,32 +54,41 @@
349
350 TEST(Test, object_to_js)
351 {
352- auto test_object = v8cpp::run_script<EmbeddedTestClass*>(
353+ auto test_object = v8cpp::run_script<std::shared_ptr<EmbeddedTestClass>>(
354+ R"(
355+ var module = require("./test-objects-module");
356+ var test_object = module.new_TestClass(1, 2);
357+ test_object.embedded_class_uptr();
358+ )");
359+
360+ EXPECT_EQ(test_object->i(), -1);
361+
362+ auto test_object2 = v8cpp::run_script<std::shared_ptr<EmbeddedTestClass>>(
363+ R"(
364+ var module = require("./test-objects-module");
365+ var test_object = module.new_TestClass(1, 2);
366+ test_object.embedded_class_sptr();
367+ )");
368+
369+ EXPECT_EQ(test_object2->i(), -1);
370+
371+ auto test_object3 = v8cpp::run_script<EmbeddedTestClass*>(
372 R"(
373 var module = require("./test-objects-module");
374 var test_object = module.new_TestClass(1, 2);
375 test_object.embedded_class_ptr();
376 )");
377
378- EXPECT_EQ(test_object->i(), -1);
379-
380- auto test_object2 = v8cpp::run_script<EmbeddedTestClass&>(
381- R"(
382- var module = require("./test-objects-module");
383- var test_object = module.new_TestClass(1, 2);
384- test_object.embedded_class_ref();
385- )");
386-
387- EXPECT_EQ(test_object2.i(), -1);
388-
389- auto test_object3 = v8cpp::run_script<EmbeddedTestClass>(
390+ EXPECT_EQ(test_object3->i(), -1);
391+
392+ auto test_object4 = v8cpp::run_script<EmbeddedTestClass>(
393 R"(
394 var module = require("./test-objects-module");
395 var test_object = module.new_TestClass(1, 2);
396 test_object.embedded_class_copy();
397 )");
398
399- EXPECT_EQ(test_object3.i(), -1);
400+ EXPECT_EQ(test_object4.i(), -1);
401 }
402
403 TEST(Test, object_from_js)
404@@ -94,9 +103,31 @@
405 test_object;
406 )");
407
408+ auto result = v8cpp::run_script<int>(
409+ R"(
410+ var module = require("./test-objects-module");
411+ var test_object = module.new_TestClass(1, 2);
412+
413+ var embedded_object = test_object.embedded_class_uptr();
414+ embedded_object.i();
415+ )");
416+
417+ EXPECT_EQ(result, -1);
418+
419+ auto result2 = v8cpp::run_script<int>(
420+ R"(
421+ var module = require("./test-objects-module");
422+ var test_object = module.new_TestClass(1, 2);
423+
424+ var embedded_object = test_object.embedded_class_sptr();
425+ embedded_object.i();
426+ )");
427+
428+ EXPECT_EQ(result2, -1);
429+
430 EXPECT_EQ(test_object.i(), -1);
431
432- auto result = v8cpp::run_script<int>(
433+ auto result3 = v8cpp::run_script<int>(
434 R"(
435 var module = require("./test-objects-module");
436 var test_object = module.new_TestClass(1, 2);
437@@ -106,5 +137,17 @@
438 test_object2.embedded_class_ref());
439 )");
440
441- EXPECT_EQ(result, -2);
442+ EXPECT_EQ(result3, -2);
443+
444+ auto result4 = v8cpp::run_script<int>(
445+ R"(
446+ var module = require("./test-objects-module");
447+ var test_object = module.new_TestClass(1, 2);
448+ var test_object2 = module.new_TestClass(1, 2);
449+
450+ test_object.add_i(test_object.embedded_class_sptr(),
451+ test_object2.embedded_class_uptr());
452+ )");
453+
454+ EXPECT_EQ(result4, -2);
455 }
456
457=== modified file 'tests/objects/test.h'
458--- tests/objects/test.h 2015-07-08 06:20:26 +0000
459+++ tests/objects/test.h 2015-09-01 09:57:53 +0000
460@@ -20,6 +20,8 @@
461
462 #include <gtest/gtest.h>
463
464+#include <memory>
465+
466 class EmbeddedTestClass
467 {
468 public:
469@@ -43,7 +45,7 @@
470 {
471 public:
472 TestClass(int a, int b)
473- : embedded_class_(a, b)
474+ : embedded_class_(std::make_shared<EmbeddedTestClass>(a, b))
475 {
476 EXPECT_EQ(a, 1);
477 EXPECT_EQ(b, 2);
478@@ -55,19 +57,29 @@
479 return i_;
480 }
481
482+ std::shared_ptr<EmbeddedTestClass> embedded_class_sptr()
483+ {
484+ return embedded_class_;
485+ }
486+
487+ std::unique_ptr<EmbeddedTestClass> embedded_class_uptr()
488+ {
489+ return std::unique_ptr<EmbeddedTestClass>(new EmbeddedTestClass(1, 2));
490+ }
491+
492 EmbeddedTestClass* embedded_class_ptr()
493 {
494- return &embedded_class_;
495+ return embedded_class_.get();
496 }
497
498 EmbeddedTestClass& embedded_class_ref()
499 {
500- return embedded_class_;
501+ return *embedded_class_;
502 }
503
504 EmbeddedTestClass embedded_class_copy()
505 {
506- return embedded_class_;
507+ return *embedded_class_;
508 }
509
510 void replace_i(EmbeddedTestClass const& other)
511@@ -81,7 +93,7 @@
512 }
513
514 private:
515- EmbeddedTestClass embedded_class_;
516+ std::shared_ptr<EmbeddedTestClass> embedded_class_;
517 int i_;
518 };
519
520
521=== modified file 'valgrind.supp'
522--- valgrind.supp 2015-07-08 09:37:32 +0000
523+++ valgrind.supp 2015-09-01 09:57:53 +0000
524@@ -21,7 +21,8 @@
525 GC not cleaning up exported objects (via to_v8)
526 Memcheck:Leak
527 fun:_Znwm
528- fun:_ZN5v8cpp8internal7Convert*to_v8EPN2v87IsolateERKS2_
529+ fun:_ZN5v8cpp8internal7Convert*to_v8*
530+ ...
531 fun:_ZN5v8cpp5to_v8*
532 fun:_ZN5v8cpp8internal14forward_invoke*
533 fun:_ZN5v8cpp8internal16forward_function*

Subscribers

People subscribed via source and target branches

to all changes: