Bug 40435 – [v8] Introduce single element caches for WebCore::String to v8::String conversions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[anton muhin]

[v8] Introduce single element caches for WebCore::String to v8::String conversions

[anton muhin]

Created attachment 58396 [details]
First version

[anton muhin]

Comment on attachment 58396 [details]
First version

I removed a check for WTF::MainThread() from getCache function.  The idea is that in that case caching should be disabled and thus this function would always return an empty map.

If you think this reasoning is too subtle, I'd be glad to revert this modification.

[Adam Barth]

Comment on attachment 58396 [details]
First version

Cool.

WebCore/bindings/v8/V8Binding.cpp: 
 +      ASSERT(WTF::isMainThread());
I didn't quite understand the reasoning behind remove this assert.  I'm not necessarily against it, I just don't understand.

WebCore/bindings/v8/V8Binding.h:84
 +      // Note: RefPtr is a must as we cache by StringImpl* equality, not identity
I don't quite understand the difference between equality and identity, but it makes sense to me why we need a RefPtr here.

[anton muhin]

Thanks a lot, Adam.

Difference between equality and identity: by identity I mean just an address of StringImpl, therefore there might be not identical, but equal string impls (if their contents is the same).

If you could suggest better words, that'd be most appreciated.

Regarding MainThread.  I just speculative fetch the map in any thread.  But I don't add anything into it if cache is not allowed (that is we're always on MainThread).  Let me roll it back as it is too complicated.

[anton muhin]

Created attachment 58488 [details]
Restore isMainThread assertion

[Adam Barth]

Comment on attachment 58488 [details]
Restore isMainThread assertion

> Regarding MainThread.  I just speculative fetch the map in any thread.

I see.  I'd have to looking into it more carefully to see whether that's safe.  If it doesn't have a big perf impact, it might be netter to leave the assert in so the code is easier to understand.

[anton muhin]

(In reply to comment #6)
> (From update of attachment 58488 [details])
> > Regarding MainThread.  I just speculative fetch the map in any thread.
> 
> I see.  I'd have to looking into it more carefully to see whether that's safe.  If it doesn't have a big perf impact, it might be netter to leave the assert in so the code is easier to understand.

Agree.  It shouldn't be big perf impact as it is a slow case anyway.  Plus condition is perfectly predictable.

Once again, thanks a lot for review, cq+'ing.

[WebKit Commit Bot]

Comment on attachment 58488 [details]
Restore isMainThread assertion

Clearing flags on attachment: 58488

Committed r61031: <http://trac.webkit.org/changeset/61031>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Created attachment 58785 [details]
Clean single element cache on major GCs which may invalidate handles

[Nate Chapin]

Comment on attachment 58785 [details]
Clean single element cache on major GCs which may invalidate handles

Ok.

[anton muhin]

As it got reverted

[anton muhin]

Comment on attachment 58785 [details]
Clean single element cache on major GCs which may invalidate handles

Clearing flags on attachment: 58785

Committed r61252: <http://trac.webkit.org/changeset/61252>

[anton muhin]

All reviewed patches have been landed.  Closing bug.