We can use ScopedPersistent for IntegerCache::smallIntegers instead of manual Persistent::New().
Created attachment 177612 [details] Patch
Comment on attachment 177612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177612&action=review > Source/WebCore/bindings/v8/V8ValueCache.h:91 > - v8::Persistent<v8::Integer> m_smallIntegers[numberOfCachedSmallIntegers]; > + ScopedPersistent<v8::Integer> m_smallIntegers[numberOfCachedSmallIntegers]; Does C++ actually call all these destructors? I guess it must.
(In reply to comment #2) > (From update of attachment 177612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177612&action=review > > > Source/WebCore/bindings/v8/V8ValueCache.h:91 > > - v8::Persistent<v8::Integer> m_smallIntegers[numberOfCachedSmallIntegers]; > > + ScopedPersistent<v8::Integer> m_smallIntegers[numberOfCachedSmallIntegers]; > > Does C++ actually call all these destructors? I guess it must. I think it should. The following code worked in my environment: class A { public: A() { printf("constructor"); } ~A() { printf("destructor"); } }; class B { public: A array[100]; }; void f() { B b; // constructors and destructors are called. } int main(void) { f(); return 0; }
Comment on attachment 177612 [details] Patch Attachment 177612 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15133624 New failing tests: fast/filesystem/workers/file-writer-sync-write-overlapped.html http/tests/filesystem/workers/resolve-url.html http/tests/security/cross-origin-worker-websql-allowed.html fast/filesystem/workers/file-writer-gc-blob.html http/tests/security/contentSecurityPolicy/worker-eval-blocked.html http/tests/filesystem/workers/resolve-url-sync.html fast/filesystem/workers/file-from-file-entry-sync.html fast/files/workers/worker-read-file-sync.html fast/filesystem/workers/file-writer-empty-blob.html fast/files/workers/worker-read-blob-async.html fast/canvas/webgl/worker-data-view-test.html fast/filesystem/workers/detached-frame-crash.html http/tests/eventsource/workers/eventsource-simple.html http/tests/security/contentSecurityPolicy/worker-connect-src-blocked.html fast/files/workers/worker-read-blob-sync.html fast/files/workers/worker-read-file-async.html fast/filesystem/workers/file-writer-events.html http/tests/security/contentSecurityPolicy/worker-connect-src-allowed.html fast/filesystem/workers/file-writer-sync-truncate-extend.html fast/filesystem/workers/file-writer-truncate-extend.html fast/filesystem/workers/file-writer-write-overlapped.html fast/filesystem/workers/file-entry-to-uri-sync.html http/tests/security/cross-origin-worker-websql.html http/tests/security/contentSecurityPolicy/worker-set-timeout-blocked.html http/tests/media/video-throttled-load-metadata.html fast/files/workers/worker-apply-blob-url-to-xhr.html fast/files/workers/inline-worker-via-blob-url.html fast/filesystem/workers/file-from-file-entry.html fast/filesystem/workers/simple-persistent-sync.html http/tests/security/contentSecurityPolicy/worker-function-function-blocked.html
Created attachment 177659 [details] fixed crashes
No isolate for us, huh?
(In reply to comment #6) > No isolate for us, huh? It looks like more work is needed to use Isolate in IntegerCache()'s constructor. If we use v8::Integer::New(value, isolate), the code crashes in workers. Maybe we need to change the initialization order around the isolate.
I doubt its a big deal.
BTW, v8Integer() is still much slower than jsNumber(). This affects performance of e.g. nodeType, which is one of the most frequently used DOM attributes in our real-world profiling. What jsNumber(value) does: u.asBits.tag = Int32Tag; // Just set a tag onto a JSValue u.asBits.payload = value; // ...and set a value. What v8Integer(value) does: Isolate* isolate = info.GetIsolate(); // Read a void* pointer located at some offset from the info. if (isolate) { V8PerIsolateData* data = isolate->GetData(); // Read a void* pointer located at some offset from the isolate. IntegerCache* cache = data->integerCache(); if (0 <= value && value < 64) { ScopedPersistent<Integer> v8Value = cache->smallIntegers[value]; return v8Value.get(); } }
Comment on attachment 177659 [details] fixed crashes Clearing flags on attachment: 177659 Committed r136638: <http://trac.webkit.org/changeset/136638>
All reviewed patches have been landed. Closing bug.
> BTW, v8Integer() is still much slower than jsNumber(). This affects performance of e.g. nodeType, which is one of the most frequently used DOM attributes in our real-world profiling. V8 should give us an API for making a SMI directly. I think it just amounts to ((value << 1) | 1) and a reinterpret cast.
(In reply to comment #12) > > BTW, v8Integer() is still much slower than jsNumber(). This affects performance of e.g. nodeType, which is one of the most frequently used DOM attributes in our real-world profiling. > > V8 should give us an API for making a SMI directly. I think it just amounts to ((value << 1) | 1) and a reinterpret cast. Yes, that sounds like a right way to fix the problem.