Bug 180916 – [JSC] Use IsoSpace for JSWeakMap and JSWeakSet to use finalizeUnconditionally

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

[Yusuke Suzuki]

[JSC] Use IsoSpace for JSWeakMap and JSWeakSet to use finalizeUnconditionally

[Yusuke Suzuki]

Created attachment 329604 [details]
Patch

[Yusuke Suzuki]

Created attachment 329615 [details]
Patch

[Darin Adler]

Comment on attachment 329615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329615&action=review

I see the performance benefit of this.

What is the performance cost? Will this use more memory or reduce locality of reference in a way that will affect speed of execution?

Is there a security benefit or a security cost?

> Source/JavaScriptCore/ChangeLog:12
> +        Currently we still have WeakReferenceHaverster in JSWeakMap and JSWeakSet. We should

typo: "harvester" rather than "haverster"

> Source/JavaScriptCore/runtime/WeakMapImplInlines.h:33
> +template <typename WeakMapBucket>

No space after "template" to match the style we prefer elsewhere.

> Source/JavaScriptCore/runtime/WeakMapImplInlines.h:34
> +inline void WeakMapImpl<WeakMapBucket>::finalizeUnconditionally(VM&)

Do we really need to mark this inline? Since it’s a function template we don’t need it for correctness, so I presume it’s needed here only because it actually convinces some particular compiler to do the inlining.

[Filip Pizlo]

(In reply to Darin Adler from comment #3)
> Comment on attachment 329615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329615&action=review
> 
> I see the performance benefit of this.
> 
> What is the performance cost? Will this use more memory or reduce locality
> of reference in a way that will affect speed of execution?

I previously found that if you have to iterate a bag of objects during GC then it’s up to 5x faster to have those objects allocated next to each other. This was an extreme case that happened in Dromaeo. Most other measurements show it to be neutral. 

> 
> Is there a security benefit or a security cost?

Security benefit. Allocating objects in isoheaps neutralizes their value for UAF-based RCE attacks. 

> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        Currently we still have WeakReferenceHaverster in JSWeakMap and JSWeakSet. We should
> 
> typo: "harvester" rather than "haverster"
> 
> > Source/JavaScriptCore/runtime/WeakMapImplInlines.h:33
> > +template <typename WeakMapBucket>
> 
> No space after "template" to match the style we prefer elsewhere.
> 
> > Source/JavaScriptCore/runtime/WeakMapImplInlines.h:34
> > +inline void WeakMapImpl<WeakMapBucket>::finalizeUnconditionally(VM&)
> 
> Do we really need to mark this inline? Since it’s a function template we
> don’t need it for correctness, so I presume it’s needed here only because it
> actually convinces some particular compiler to do the inlining.

We put these in the same translation unit as their caller by having them be in an Inlines easer included from Heap.cpp. It would be ugly to define this knowing heap.cpp. I don’t know how profitable inlining is here, but we usually err on the side of giving the compiler an option to do it for GC critical paths.

[Darin Adler]

(In reply to Filip Pizlo from comment #4)
> (In reply to Darin Adler from comment #3)
> > Do we really need to mark this inline? Since it’s a function template we
> > don’t need it for correctness, so I presume it’s needed here only because it
> > actually convinces some particular compiler to do the inlining.
> 
> We put these in the same translation unit as their caller by having them be
> in an Inlines easer included from Heap.cpp. It would be ugly to define this
> knowing heap.cpp. I don’t know how profitable inlining is here, but we
> usually err on the side of giving the compiler an option to do it for GC
> critical paths.

My proposal is: leave this in a WeakMapImplInlines.h header and let the compiler inline it when compiling Heap.cpp. But do not explicitly mark it with the inline keyword because it’s a template and so we don’t need to.

I’m not sure my proposal is right, but it doesn’t necessarily interfere with anything you mentioned above.

[Filip Pizlo]

(In reply to Darin Adler from comment #5)
> (In reply to Filip Pizlo from comment #4)
> > (In reply to Darin Adler from comment #3)
> > > Do we really need to mark this inline? Since it’s a function template we
> > > don’t need it for correctness, so I presume it’s needed here only because it
> > > actually convinces some particular compiler to do the inlining.
> > 
> > We put these in the same translation unit as their caller by having them be
> > in an Inlines easer included from Heap.cpp. It would be ugly to define this
> > knowing heap.cpp. I don’t know how profitable inlining is here, but we
> > usually err on the side of giving the compiler an option to do it for GC
> > critical paths.
> 
> My proposal is: leave this in a WeakMapImplInlines.h header and let the
> compiler inline it when compiling Heap.cpp. But do not explicitly mark it
> with the inline keyword because it’s a template and so we don’t need to.
> 
> I’m not sure my proposal is right, but it doesn’t necessarily interfere with
> anything you mentioned above.

Oh!  Yes, you're right.  I missed the part about it being a template.

[Yusuke Suzuki]

Comment on attachment 329615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=329615&action=review

Thanks!

>>> Source/JavaScriptCore/ChangeLog:12
>>> +        Currently we still have WeakReferenceHaverster in JSWeakMap and JSWeakSet. We should
>> 
>> typo: "harvester" rather than "haverster"
> 
> We put these in the same translation unit as their caller by having them be in an Inlines easer included from Heap.cpp. It would be ugly to define this knowing heap.cpp. I don’t know how profitable inlining is here, but we usually err on the side of giving the compiler an option to do it for GC critical paths.

Oops, fixed.

>> Source/JavaScriptCore/runtime/WeakMapImplInlines.h:33
>> +template <typename WeakMapBucket>
> 
> No space after "template" to match the style we prefer elsewhere.

Fixed.

>> Source/JavaScriptCore/runtime/WeakMapImplInlines.h:34
>> +inline void WeakMapImpl<WeakMapBucket>::finalizeUnconditionally(VM&)
> 
> Do we really need to mark this inline? Since it’s a function template we don’t need it for correctness, so I presume it’s needed here only because it actually convinces some particular compiler to do the inlining.

OK, drop this "inline" keyword, and let compiler decide inlining.

[Yusuke Suzuki]

Committed r226017: <https://trac.webkit.org/changeset/226017>

[Radar WebKit Bug Importer]

<rdar://problem/36099376>