Bug 70258 - Simplified GC marking logic
Summary: Simplified GC marking logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-17 13:01 PDT by Geoffrey Garen
Modified: 2011-10-17 13:42 PDT (History)
3 users (show)

See Also:


Attachments
Patch (28.27 KB, patch)
2011-10-17 13:18 PDT, Geoffrey Garen
fpizlo: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2011-10-17 13:01:59 PDT
Simplified GC marking logic
Comment 1 Geoffrey Garen 2011-10-17 13:18:52 PDT
Created attachment 111308 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-17 13:21:22 PDT
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 3 Darin Adler 2011-10-17 13:25:30 PDT
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?
Comment 4 Geoffrey Garen 2011-10-17 13:32:14 PDT
> > 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 5 Early Warning System Bot 2011-10-17 13:40:18 PDT
Comment on attachment 111308 [details]
Patch

Attachment 111308 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10106150
Comment 6 Geoffrey Garen 2011-10-17 13:42:41 PDT
Committed r97642: <http://trac.webkit.org/changeset/97642>