Summary: | Simplified GC marking logic | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | fpizlo, oliver, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Geoffrey Garen
2011-10-17 13:01:59 PDT
Created attachment 111308 [details]
Patch
Attachment 111308 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style. Comment on attachment 111308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111308&action=review > Source/JavaScriptCore/heap/MarkStack.cpp:42 > + , m_capacity(m_allocated / sizeof(JSCell*)) This uses an uninitialized m_allocated data member. > Source/JavaScriptCore/heap/MarkStack.cpp:72 > +#if OS(WINDOWS) > + // We cannot release a part of a region with VirtualFree. To get around this, > + // we'll release the entire region and reallocate the size that we want. Can we add a tiny bit of abstraction here so the #if OS (WINDOWS) isn’t right inline in the code? This knowledge of what MarkStack::releaseStack can and can’t do is now in a different class from the releaseStack function. > Source/JavaScriptCore/heap/MarkStack.cpp:118 > + cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), visitor); Looks like you removed m_isCheckingForDefaultMarkViolation here. Why? > > Source/JavaScriptCore/heap/MarkStack.cpp:42 > > + , m_capacity(m_allocated / sizeof(JSCell*)) > > This uses an uninitialized m_allocated data member. Oops! > > Source/JavaScriptCore/heap/MarkStack.cpp:72 > > +#if OS(WINDOWS) > > + // We cannot release a part of a region with VirtualFree. To get around this, > > + // we'll release the entire region and reallocate the size that we want. > > Can we add a tiny bit of abstraction here so the #if OS (WINDOWS) isn’t right inline in the code? This knowledge of what MarkStack::releaseStack can and can’t do is now in a different class from the releaseStack function. Yes. I think this can move into OSAllocator. > > Source/JavaScriptCore/heap/MarkStack.cpp:118 > > + cell->methodTable()->visitChildren(const_cast<JSCell*>(cell), visitor); > > Looks like you removed m_isCheckingForDefaultMarkViolation here. Why? There's no such thing as a "default mark violation" anymore. I'm going to remove the rest of it, include the structure flag, in a follow-up patch. Comment on attachment 111308 [details] Patch Attachment 111308 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10106150 Committed r97642: <http://trac.webkit.org/changeset/97642> |