Implement Node map in intrusive way
Created attachment 47126 [details] Patch
(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.
Attachment 47126 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/203247
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.
By Vector, I mean HashMap.
(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.
Created attachment 47201 [details] Addressing stylistic issues and rebaselining
(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.
(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.
Created attachment 47372 [details] Refactoring underlying storage into ChunkedTable
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.
Created attachment 47409 [details] Addressing Adam comments
Comment on attachment 47409 [details] Addressing Adam comments thanks
Created attachment 47410 [details] Patch for landing
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
Created attachment 47521 [details] Patch for landing
Comment on attachment 47521 [details] Patch for landing Clearing flags on attachment: 47521 Committed r53944: <http://trac.webkit.org/changeset/53944>
All reviewed patches have been landed. Closing bug.