[Webkit-unassigned] [Bug 70995] The GC should be parallel

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=70995





--- Comment #29 from Filip Pizlo <fpizlo at apple.com>  2011-10-31 18:22:14 PST ---

> > Source/JavaScriptCore/heap/MarkStack.h:311
> > +    inline void MarkStack::addOpaqueRoot(void* root)
> >      {
> > -        return m_opaqueRoots.add(root).second;
> > +#if ENABLE(PARALLEL_GC)
> > +        mergeOpaqueRootsIfProfitable();
> > +#endif
> > +        m_opaqueRoots.add(root);
> >      }
> 
> I'm surprised that's profitable to merge opaque roots eagerly. Nothing can be done with the set of opaque roots until the end of parallel work, anyway. Could be simpler just to omit eager merging.
> 
> Do you know why this is profitable?

It's not about profitability in the execution time sense.  It's profitability in the space usage sense.  If we did happen to have a lot of opaque roots, then we don't want the worst case of each worker thread holding onto its copy of the same hash set.  So merging when it gets big ensures that we minimize space usage.

I also have a call to mergeIfNecessary() in drain(), for a plainly dumb reason: the threads currently do not know about GC phase because whenever they go into idle state they will have merged all of their information into the shared data.  That keeps things simple.

> 
> > Source/JavaScriptCore/runtime/Heuristics.cpp:32
> > +#if PLATFORM(MAC) && ENABLE(PARALLEL_GC)
> 
> OS(UNIX) or OS(DARWIN) is better than PLATFORM(MAC) here.

OS(DARWIN) probably, since this particular incantation of sysctl is missing on some unixes.

> 
> > Source/JavaScriptCore/runtime/Heuristics.cpp:177
> > +#if PLATFORM(MAC) && ENABLE(PARALLEL_GC)
> > +    int name[2];
> > +    size_t valueSize = sizeof(cpusToUse);
> > +    name[0] = CTL_HW;
> > +    name[1] = HW_AVAILCPU;
> > +    sysctl(name, 2, &cpusToUse, &valueSize, 0, 0);
> 
> OS(UNIX) or OS(DARWIN) is better than PLATFORM(MAC) here.
> 
> > Source/JavaScriptCore/wtf/Bitmap.h:40
> > +#if PLATFORM(MAC)
> 
> CPU(X86) is better than PLATFORM(MAC) here.

(CPU(X86) || CPU(X86_64)) && OS(UNIX) at least, right?  Or some manner of check for whether the compiler supports GCC-style inline asm?

> 
> > Source/JavaScriptCore/wtf/Platform.h:1078
> > +#if !ENABLE(PARALLEL_GC) && PLATFORM(MAC)
> > +#define ENABLE_PARALLEL_GC 1
> 
> This should be "#!defined(ENABLE_PARALLEL_GC)" instead of "!ENABLE(PARALLEL_GC)" (so you can manually define to disable parallel GC).

Oops!  And I guess this should also be guarded with some variant of CPU(X86) to get it to work on PPC.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.