Bug 70258 – Simplified GC marking logic

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Simplified GC marking logic

[Geoffrey Garen]

Created attachment 111308 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

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?

[Geoffrey Garen]

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

[Early Warning System Bot]

Comment on attachment 111308 [details]
Patch

Attachment 111308 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10106150

[Geoffrey Garen]

Committed r97642: <http://trac.webkit.org/changeset/97642>