Bug 104066 – [V8] Use ScopedPersistent for IntegerCache::smallIntegers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

We can use ScopedPersistent for IntegerCache::smallIntegers instead of manual Persistent::New().

[Kentaro Hara]

Created attachment 177612 [details]
Patch

[Adam Barth]

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.

[Kentaro Hara]

(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;
}

[WebKit Review Bot]

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

[Kentaro Hara]

Created attachment 177659 [details]
fixed crashes

[Adam Barth]

No isolate for us, huh?

[Kentaro Hara]

(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.

[Adam Barth]

I doubt its a big deal.

[Kentaro Hara]

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();
    }
  }

[WebKit Review Bot]

Comment on attachment 177659 [details]
fixed crashes

Clearing flags on attachment: 177659

Committed r136638: <http://trac.webkit.org/changeset/136638>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> 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.

[Kentaro Hara]

(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.