Bug 33957 - [v8] Implement Node map in intrusive way for better speed
Summary: [v8] Implement Node map in intrusive way for better speed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-21 09:07 PST by anton muhin
Modified: 2010-01-27 11:18 PST (History)
7 users (show)

See Also:


Attachments
Patch (18.04 KB, patch)
2010-01-21 09:08 PST, anton muhin
no flags Details | Formatted Diff | Diff
Addressing stylistic issues and rebaselining (18.07 KB, patch)
2010-01-22 06:59 PST, anton muhin
no flags Details | Formatted Diff | Diff
Refactoring underlying storage into ChunkedTable (18.70 KB, patch)
2010-01-25 15:11 PST, anton muhin
no flags Details | Formatted Diff | Diff
Addressing Adam comments (18.73 KB, patch)
2010-01-26 07:02 PST, anton muhin
no flags Details | Formatted Diff | Diff
Patch for landing (18.73 KB, patch)
2010-01-26 07:20 PST, anton muhin
no flags Details | Formatted Diff | Diff
Patch for landing (18.67 KB, patch)
2010-01-27 05:27 PST, 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-01-21 09:07:36 PST
Implement Node map in intrusive way
Comment 1 anton muhin 2010-01-21 09:08:49 PST
Created attachment 47126 [details]
Patch
Comment 2 anton muhin 2010-01-21 09:12:13 PST
(In reply to comment #1)
> Created an attachment (id=47126) [details]
> Patch

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=32430 so all the builds should fail.  I would rerun EWSes when other patch is submitted.
Comment 3 WebKit Review Bot 2010-01-21 10:19:06 PST
Attachment 47126 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/203247
Comment 4 Adam Barth 2010-01-21 16:28:56 PST
Comment on attachment 47126 [details]
Patch

+ if (!wrapper) return v8::Persistent<v8::Object>();

Should be two lines.  (This nit repeated many times.)

+ TableChunk

Why not just use Vector?

+ virtual v8::Persistent<ValueType> get(KeyType* obj)

Are these methods already virtual?  It seems like we shouldn't need that.  We should be able to use templates to solve the dispatch problem at compile time.

r- for style, but curious about the above questions.
Comment 5 Adam Barth 2010-01-21 17:34:31 PST
By Vector, I mean HashMap.
Comment 6 anton muhin 2010-01-22 06:30:42 PST
(In reply to comment #4)
> (From update of attachment 47126 [details])
> + if (!wrapper) return v8::Persistent<v8::Object>();
> 
> Should be two lines.  (This nit repeated many times.)

Hopefully fixed all those, sorry.  Just curious, how difficult it'd be to add this check into style-checking script?

> + TableChunk
> 
> Why not just use Vector?

I am not sure I fully understand you.  You later corrected it to HashMap.  Anyway, one thing I'd like to escape is x2 allocation policy used by wtf's HashTable and Vector for two reasons: less memory and speed.

I know one deficiency of my current implementation: it thrashes when on adds and removes the element which could lead to allocation and deallocation of next chunk.  It's quite easy to fix, but would complicate the code slightly.  What do you think, should I fix it immediately?

> + virtual v8::Persistent<ValueType> get(KeyType* obj)
> 
> Are these methods already virtual?  It seems like we shouldn't need that.  We
> should be able to use templates to solve the dispatch problem at compile time.

I think they already are---WeakReferenceMap declares all those methods as virtual.

I wholeheartedly agree with you that we should at least try to de-virtualize them though, but could we save it for another CL?

Running through tests on my box and uploading new patch.  Thanks a lot for comments, Adam.
Comment 7 anton muhin 2010-01-22 06:59:06 PST
Created attachment 47201 [details]
Addressing stylistic issues and rebaselining
Comment 8 David Levin 2010-01-22 08:46:41 PST
(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 47126 [details] [details])
> > + if (!wrapper) return v8::Persistent<v8::Object>();
> > 
> > Should be two lines.  (This nit repeated many times.)
> 
> Hopefully fixed all those, sorry.  Just curious, how difficult it'd be to add
> this check into style-checking script?

Simply an issue of figuring out an appropriate regex/code to detect this and putting it in the style checking script (along with an appropriate error category and test). The hardest part is the correct detection (which may not be too hard). If you'd like to do it, that would be great, imo.
Comment 9 anton muhin 2010-01-25 11:25:45 PST
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 47126 [details] [details] [details])
> > > + if (!wrapper) return v8::Persistent<v8::Object>();
> > > 
> > > Should be two lines.  (This nit repeated many times.)
> > 
> > Hopefully fixed all those, sorry.  Just curious, how difficult it'd be to add
> > this check into style-checking script?
> 
> Simply an issue of figuring out an appropriate regex/code to detect this and
> putting it in the style checking script (along with an appropriate error
> category and test). The hardest part is the correct detection (which may not be
> too hard). If you'd like to do it, that would be great, imo.

Thanks, David, I think I found the place.  Let me try to hack it in.
Comment 10 anton muhin 2010-01-25 15:11:47 PST
Created attachment 47372 [details]
Refactoring underlying storage into ChunkedTable
Comment 11 Adam Barth 2010-01-25 16:21:24 PST
Comment on attachment 47372 [details]
Refactoring underlying storage into ChunkedTable

+ m_table()

You don't need to call the default constructor explicitly.

+ static int const kNoEntries = (1 << 10) - 1;

Perhaps numberOfEntries?  (WebKit-land doesn't use "k" and the No is confusing).

Thanks for factoring ChunkedTable out.  It's much easier to follow what's going on abstractly.  As we discussed, we should move ChunkedTable to a more generic location in a future patch.

Looks great!  I wish we had some unit testing for ChunkedTable, but that's not the way WebKit rolls.
Comment 12 anton muhin 2010-01-26 07:02:25 PST
Created attachment 47409 [details]
Addressing Adam comments
Comment 13 Adam Barth 2010-01-26 07:14:45 PST
Comment on attachment 47409 [details]
Addressing Adam comments

thanks
Comment 14 anton muhin 2010-01-26 07:20:06 PST
Created attachment 47410 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2010-01-27 05:17:41 PST
Comment on attachment 47410 [details]
Patch for landing

Rejecting patch 47410 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
/V8DOMMap.cpp
	M	WebCore/bindings/v8/V8DOMMap.h
	M	WebCore/bindings/v8/V8DOMWrapper.cpp
	M	WebCore/bindings/v8/V8DOMWrapper.h
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/214946
Comment 16 anton muhin 2010-01-27 05:27:34 PST
Created attachment 47521 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2010-01-27 11:18:35 PST
Comment on attachment 47521 [details]
Patch for landing

Clearing flags on attachment: 47521

Committed r53944: <http://trac.webkit.org/changeset/53944>
Comment 18 WebKit Commit Bot 2010-01-27 11:18:43 PST
All reviewed patches have been landed.  Closing bug.