Bug 212585 – Change Gigacage::Config to use storage in WebConfig::g_config instead of its own.

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Mark Lam]

This should save us another 16K for LuaJSFight.

[Radar WebKit Bug Importer]

<rdar://problem/63812487>

[Mark Lam]

Created attachment 400710 [details]
proposed patch.

[Mark Lam]

Created attachment 400711 [details]
proposed patch.

[Mark Lam]

Created attachment 400712 [details]
proposed patch.

[Mark Lam]

Created attachment 400713 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 400714 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 400715 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 400720 [details]
proposed patch.

[Mark Lam]

Created attachment 400721 [details]
proposed patch.

[Mark Lam]

Looks like Win EWS bot failure is unrelated to my patch.  That means this is ready for a review.

[Yusuke Suzuki]

Comment on attachment 400721 [details]
proposed patch.

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

> Source/WTF/ChangeLog:24
> +        We now think of the various Config records as being allocated from parts of a
> +        WebConfig::g_config buffer.  WTF::Config will manage the mechanics of freezing
> +        that buffer.  And the JSC VM is still the determiner of if/when to freeze the
> +        buffer, and it will do this at the end of the construction of the very first
> +        VM instance (as before).
> +
> +        Gigacage::Config reserves space in WebConfig::g_config.
> +        WTF::Config will honor that reservation and place itself after that.
> +        JSC::Config will continue to place itself at WTF::Config::spaceForExtensions.
> +
> +        The upside of this approach this is that we can now share the same memory page
> +        for all the Configs, and can freeze them in one go.
> +
> +        The downside is that g_gigacageConfig, g_wtfConfig, and g_jscConfig now have to
> +        be macros.  This results in some weirdness e.g. they are no longer qualified by
> +        namespaces: referring to WTF::g_wtfConfig is now incorrect.

This is unfortunate that we need to organize this as it is. However, given that RAMification and daemon apps' requirement is super tight, we have no room for wasting 16KB memory. Actually, this improves LuaJSFight by 2%.
So, I agree to this direction.

> Source/bmalloc/bmalloc/Gigacage.cpp:202
>  }

Does disablePrimitiveGigacage work after this change? If it does not work, do we need to have this function?
This function is called from ArrayBuffer::createFromBytes, but it is super likely that this function is called after the config is permanently frozen.

[Mark Lam]

*** Bug 212599 has been marked as a duplicate of this bug. ***

[Mark Lam]

(In reply to Yusuke Suzuki from comment #11)
> Does disablePrimitiveGigacage work after this change? If it does not work,
> do we need to have this function?
> This function is called from ArrayBuffer::createFromBytes, but it is super
> likely that this function is called after the config is permanently frozen.

You are correct.  Unconditional freezing does break disablePrimitiveGigacage().  I've implemented a workaround for this in my revised patch.  Uploading soon for testing.

[Mark Lam]

Created attachment 400792 [details]
proposed patch.

[Guillaume Emont]

Comment on attachment 400792 [details]
proposed patch.

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

> Source/bmalloc/bmalloc/Gigacage.h:216
> +BINLINE bool disablingPrimitiveGigacageIsForbidden() { return false }

Looks like the missing semicolon is what's making the 32-bit EWS bots red.

[Mark Lam]

Created attachment 400813 [details]
propose patch.

[Mark Lam]

Comment on attachment 400813 [details]
propose patch.

I think this is ready for a review now.

[Yusuke Suzuki]

Comment on attachment 400813 [details]
propose patch.

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

r=me

> Source/bmalloc/ChangeLog:36
> +           base pointer.  We can longer do that because the base pointer will be frozen

/longer/no longer/

[Mark Lam]

Thanks for the review.  Landed in r262434: <http://trac.webkit.org/r262434>.