Bug 40435 - [v8] Introduce single element caches for WebCore::String to v8::String conversions
Summary: [v8] Introduce single element caches for WebCore::String to v8::String conver...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on: 40539
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-10 11:43 PDT by anton muhin
Modified: 2010-06-16 05:43 PDT (History)
4 users (show)

See Also:


Attachments
First version (4.81 KB, patch)
2010-06-10 11:47 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Restore isMainThread assertion (4.48 KB, patch)
2010-06-11 11:09 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Clean single element cache on major GCs which may invalidate handles (5.35 KB, patch)
2010-06-15 09:30 PDT, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2010-06-10 11:43:51 PDT
[v8] Introduce single element caches for WebCore::String to v8::String conversions
Comment 1 anton muhin 2010-06-10 11:47:26 PDT
Created attachment 58396 [details]
First version
Comment 2 anton muhin 2010-06-10 11:48:54 PDT
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.
Comment 3 Adam Barth 2010-06-11 10:58:37 PDT
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.
Comment 4 anton muhin 2010-06-11 11:04:57 PDT
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.
Comment 5 anton muhin 2010-06-11 11:09:22 PDT
Created attachment 58488 [details]
Restore isMainThread assertion
Comment 6 Adam Barth 2010-06-11 11:13:26 PDT
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.
Comment 7 anton muhin 2010-06-11 11:15:33 PDT
(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.
Comment 8 WebKit Commit Bot 2010-06-11 12:11:53 PDT
Comment on attachment 58488 [details]
Restore isMainThread assertion

Clearing flags on attachment: 58488

Committed r61031: <http://trac.webkit.org/changeset/61031>
Comment 9 WebKit Commit Bot 2010-06-11 12:12:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 anton muhin 2010-06-15 09:30:30 PDT
Created attachment 58785 [details]
Clean single element cache on major GCs which may invalidate handles
Comment 11 Nate Chapin 2010-06-15 10:09:56 PDT
Comment on attachment 58785 [details]
Clean single element cache on major GCs which may invalidate handles

Ok.
Comment 12 anton muhin 2010-06-15 10:17:18 PDT
As it got reverted
Comment 13 anton muhin 2010-06-16 05:43:22 PDT
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>
Comment 14 anton muhin 2010-06-16 05:43:34 PDT
All reviewed patches have been landed.  Closing bug.