Bug 33957 – [v8] Implement Node map in intrusive way for better speed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[anton muhin]

Implement Node map in intrusive way

[anton muhin]

Created attachment 47126 [details]
Patch

[anton muhin]

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

[WebKit Review Bot]

Attachment 47126 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/203247

[Adam Barth]

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.

[Adam Barth]

By Vector, I mean HashMap.

[anton muhin]

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

[anton muhin]

Created attachment 47201 [details]
Addressing stylistic issues and rebaselining

[David Levin]

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

[anton muhin]

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

[anton muhin]

Created attachment 47372 [details]
Refactoring underlying storage into ChunkedTable

[Adam Barth]

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.

[anton muhin]

Created attachment 47409 [details]
Addressing Adam comments

[Adam Barth]

Comment on attachment 47409 [details]
Addressing Adam comments

thanks

[anton muhin]

Created attachment 47410 [details]
Patch for landing

[WebKit Commit Bot]

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

[anton muhin]

Created attachment 47521 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 47521 [details]
Patch for landing

Clearing flags on attachment: 47521

Committed r53944: <http://trac.webkit.org/changeset/53944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.