[webkit-changes] Fwd: Re: [51457] trunk/JavaScriptCore

[Sam Weinig]

I can't tell from your response.  Did you or or did you not test the change
for a performance regression? If the assertions are not adding anything,
please remove them, otherwise, please replace them with STATIC_ASSERTs

-Sam

On Sat, Nov 28, 2009 at 11:56 AM, Zoltan Herczeg
<zherczeg at inf.u-szeged.hu>wrote:

> Hi Sam,
>
> (the other) Zoltan forwarded this mail to me. Let me explain what is
> happening there. The CollectorBlock contains many CollectorBlockCells, and
> its total size is ~270k on 32 bit machines (doubles on 64 bit). To get the
> vptrs of the objects, we map them to a memory space with placement new.
> However, the storage requirement for this is ONE cell, which is only 32
> bytes (!).
>
> Allocating ~270k is way faster with FastMalloc, because we only write the
> first 32 bytes, therefore the kernel will not map the whole memory space,
> only one 4k page (kernel is clever in this case). However, in case of a
> stack, the kernel will map all ~270k (allocating several 4k pages for the
> process).
>
> The story is different for 32 byte allocations. In that case registering
> the allocation with any kind of malloc is slower than allocating it on the
> stack.
>
> I think the original author also intended to use CollectorBlockCell
> (perhaps it was too late when he wrote the code :) ). That is why he was
> surprised about the slowdown. And that is why Gavin said 'lol' in the bug
> report. Do you collect funny WebKit bugs somewhere?
>
> STATIC_ASSERTS are ok as well. Actually we don't even need asserts here,
> because all JSCell checks whether they fit to a CollectorCell somewhere
> else in the code. It is just an extra protection.
>
> Zoltan
>
> > On Saturday 28 November 2009, at 17:17, Sam Weinig wrote:
> >> Can these ASSERTS be STATIC_ASSERTS?  Did you test the performance?
> >> The comment indicates the malloc was needed but there is no
> >> information in the changelog to indicate that is no longer true.
> >>
> >> - Sam
> >>
> >> On Nov 28, 2009, at 5:31 AM, zoltan at webkit.org wrote:
> >> > Revision
> >> > 51457
> >> > Author
> >> > zoltan at webkit.org
> >> > Date
> >> > 2009-11-28 02:31:18 -0800 (Sat, 28 Nov 2009)
> >> > Log Message
> >> >
> >> > 2009-11-28  Zoltan Herczeg  <zherczeg at inf.u-szeged.hu>
> >> >
> >> >         Reviewed by Gavin Barraclough.
> >> >
> >> >         https://bugs.webkit.org/show_bug.cgi?id=31930
> >> >
> >> >         Seems a typo. We don't need ~270k memory to determine the
> >> > vptrs.
> >> >
> >> >         * runtime/JSGlobalData.cpp:
> >> >         (JSC::VPtrSet::VPtrSet):
> >> > Modified Paths
> >> >
> >> > trunk/JavaScriptCore/ChangeLog
> >> > trunk/JavaScriptCore/runtime/JSGlobalData.cpp
> >> > Diff
> >> >
> >> > Modified: trunk/JavaScriptCore/ChangeLog (51456 => 51457)
> >> >
> >> > --- trunk/JavaScriptCore/ChangeLog 2009-11-28 07:39:12 UTC (rev 51456)
> >> > +++ trunk/JavaScriptCore/ChangeLog 2009-11-28 10:31:18 UTC (rev 51457)
> >> > @@ -1,3 +1,14 @@
> >> > +2009-11-28  Zoltan Herczeg  <zherczeg at inf.u-szeged.hu>
> >> > +
> >> > +        Reviewed by Gavin Barraclough.
> >> > +
> >> > +        https://bugs.webkit.org/show_bug.cgi?id=31930
> >> > +
> >> > +        Seems a typo. We don't need ~270k memory to determine the
> >> > vptrs.
> >> > +
> >> > +        * runtime/JSGlobalData.cpp:
> >> > +        (JSC::VPtrSet::VPtrSet):
> >> > +
> >> >  2009-11-27  Shinichiro Hamaji  <hamaji at chromium.org>
> >> >
> >> >          Unreviewed.
> >> > Modified: trunk/JavaScriptCore/runtime/JSGlobalData.cpp (51456 =>
> >> > 51457)
> >> >
> >> > --- trunk/JavaScriptCore/runtime/JSGlobalData.cpp  2009-11-28
> >> > 07:39:12 UTC (rev 51456)
> >> > +++ trunk/JavaScriptCore/runtime/JSGlobalData.cpp  2009-11-28
> >> > 10:31:18 UTC (rev 51457)
> >> > @@ -90,26 +90,28 @@
> >> >
> >> >  VPtrSet::VPtrSet()
> >> >  {
> >> > -    // Bizarrely, calling fastMalloc here is faster than allocating
> >> > space on the stack.
> >> > -    void* storage = fastMalloc(sizeof(CollectorBlock));
> >> > +    CollectorCell cell;
> >> > +    void* storage = &cell;
> >> >
> >> > +    ASSERT(sizeof(JSArray) <= sizeof(CollectorCell));
> >> >      JSCell* jsArray = new (storage) JSArray(JSArray::createStructure
> >> > (jsNull()));
> >> >      jsArrayVPtr = jsArray->vptr();
> >> >      jsArray->~JSCell();
> >> >
> >> > +    ASSERT(sizeof(JSByteArray) <= sizeof(CollectorCell));
> >> >      JSCell* jsByteArray = new (storage) JSByteArray
> >> > (JSByteArray::VPtrStealingHack);
> >> >      jsByteArrayVPtr = jsByteArray->vptr();
> >> >      jsByteArray->~JSCell();
> >> >
> >> > +    ASSERT(sizeof(JSString) <= sizeof(CollectorCell));
> >> >      JSCell* jsString = new (storage) JSString
> >> > (JSString::VPtrStealingHack);
> >> >      jsStringVPtr = jsString->vptr();
> >> >      jsString->~JSCell();
> >> >
> >> > +    ASSERT(sizeof(JSFunction) <= sizeof(CollectorCell));
> >> >      JSCell* jsFunction = new (storage) JSFunction
> >> > (JSFunction::createStructure(jsNull()));
> >> >      jsFunctionVPtr = jsFunction->vptr();
> >> >      jsFunction->~JSCell();
> >> > -
> >> > -    fastFree(storage);
> >> >  }
> >> >
> >> >  JSGlobalData::JSGlobalData(bool isShared, const VPtrSet& vptrSet)
> >> > _______________________________________________
> >> > webkit-changes mailing list
> >> > webkit-changes at lists.webkit.org
> >> > http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-changes/attachments/20091128/37ee89b6/attachment.html>