Transforming PersistentHandleVisitor to not need to copy Persistent handles. (issue 15974006)

[dca...@chromium.org]

lgtm

https://codereview.chromium.org/15974006/

[dca...@chromium.org]

lgtm


https://codereview.chromium.org/15974006/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15974006/diff/1/src/api.cc#newcode5111
src/api.cc:5111: Persistent<Value> handle =
ToApi<Value>(i::Handle<i::Object>(p));
can you make this a Value* and do some casting magic here?

https://codereview.chromium.org/15974006/

[ma...@chromium.org]

Reviewers: dcarney, svenpanne,

Message:
svenpanne, ptal

Description:
Transforming PersistentHandleVisitor to not need to copy Persistent handles.

This gets rid of more places where Persistent handles are copied
(see crbug.com/236290 ).

Transition plan: after this CL, Blink will be modified to work both with and
without the #define, then the #define will be removed from V8.

The corresponding Blink side changes are in
https://codereview.chromium.org/15670010/ .

BUG=

Please review this at https://codereview.chromium.org/15974006/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M include/v8.h
M src/api.cc
M test/cctest/test-api.cc


Index: include/v8.h
diff --git a/include/v8.h b/include/v8.h
index
51040fe1a70f57d992f08d6107e8f0316de42955..c7c1618951bf6c790d392d13e6f8d3c7ca5583e4
100644
--- a/include/v8.h
+++ b/include/v8.h
@@ -228,6 +228,7 @@ typedef void (*NearDeathCallback)(Isolate* isolate,


#define V8_USE_UNSAFE_HANDLES
+#define V8_USE_OLD_STYLE_PERSISTENT_HANDLE_VISITORS

/**
* An object reference managed by the v8 garbage collector.
@@ -4107,8 +4108,13 @@ class V8EXPORT ExternalResourceVisitor { // NOLINT
class V8EXPORT PersistentHandleVisitor { // NOLINT
public:
virtual ~PersistentHandleVisitor() {}
+#ifdef V8_USE_OLD_STYLE_PERSISTENT_HANDLE_VISITORS
virtual void VisitPersistentHandle(Persistent<Value> value,
uint16_t class_id) {}
+#else
+ virtual void VisitPersistentHandle(Persistent<Value>* value,
+ uint16_t class_id) {}
+#endif
};


Index: src/api.cc
diff --git a/src/api.cc b/src/api.cc
index
a5ef0543b5579cf87a5cba52f37bbe82c161bc16..68569a513e203e7c8b75c36e5ede2b8d45a872c7
100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -5104,8 +5104,14 @@ class VisitorAdapter : public i::ObjectVisitor {
UNREACHABLE();
}
virtual void VisitEmbedderReference(i::Object** p, uint16_t class_id) {
+#ifdef V8_USE_OLD_STYLE_PERSISTENT_HANDLE_VISITORS
visitor_->VisitPersistentHandle(ToApi<Value>(i::Handle<i::Object>(p)),
class_id);
+#else
+ Value* value = ToApi<Value>(i::Handle<i::Object>(p));
+ visitor_->VisitPersistentHandle(
+ reinterpret_cast<Persistent<Value>*>(&value), class_id);
+#endif
}
private:
PersistentHandleVisitor* visitor_;
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
e52dde37c994fea3d04b3444bb5cc8a6cf8d0836..0e10194020bd7ccc7019618dafd3b80ee4bc320b
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -17785,12 +17785,19 @@ class Visitor42 : public
v8::PersistentHandleVisitor {
explicit Visitor42(v8::Persistent<v8::Object> object)
: counter_(0), object_(object) { }

+#ifdef V8_USE_OLD_STYLE_PERSISTENT_HANDLE_VISITORS
virtual void VisitPersistentHandle(Persistent<Value> value,
uint16_t class_id) {
+ VisitPersistentHandle(&value, class_id);
+ }
+#endif
+
+ virtual void VisitPersistentHandle(Persistent<Value>* value,
+ uint16_t class_id) {
if (class_id == 42) {
- CHECK(value->IsObject());
+ CHECK((*value)->IsObject());
v8::Persistent<v8::Object> visited =
- v8::Persistent<v8::Object>::Cast(value);
+ v8::Persistent<v8::Object>::Cast(*value);
CHECK_EQ(42, visited.WrapperClassId(v8::Isolate::GetCurrent()));
CHECK_EQ(Handle<Value>(*object_), Handle<Value>(*visited));
++counter_;

[ma...@chromium.org]

(comment done)

https://codereview.chromium.org/15974006/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15974006/diff/1/src/api.cc#newcode5111

src/api.cc:5111: Persistent<Value> handle =

ToApi<Value>(i::Handle<i::Object>(p));

On 2013/05/28 12:58:11, dcarney wrote:
> can you make this a Value* and do some casting magic here?

Done.

https://codereview.chromium.org/15974006/

[sven...@chromium.org]

lgtm



https://codereview.chromium.org/15974006/

[dca...@chromium.org]

Committed patchset #2 manually as r14871 (presubmit successful).

https://codereview.chromium.org/15974006/