Bug 212585 - Change Gigacage::Config to use storage in WebConfig::g_config instead of its own.
Summary: Change Gigacage::Config to use storage in WebConfig::g_config instead of its ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
: 212599 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-31 16:18 PDT by Mark Lam
Modified: 2020-06-02 12:45 PDT (History)
16 users (show)

See Also:


Attachments
proposed patch. (27.91 KB, patch)
2020-05-31 17:41 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (28.11 KB, patch)
2020-05-31 17:54 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (28.26 KB, patch)
2020-05-31 18:05 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (28.77 KB, patch)
2020-05-31 18:29 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (28.79 KB, patch)
2020-05-31 18:37 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress for EWS testing. (28.80 KB, patch)
2020-05-31 18:40 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (28.81 KB, patch)
2020-05-31 21:41 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (28.82 KB, patch)
2020-05-31 22:10 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (42.51 KB, patch)
2020-06-02 00:58 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
propose patch. (42.52 KB, patch)
2020-06-02 06:43 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-05-31 16:18:17 PDT
This should save us another 16K for LuaJSFight.
Comment 1 Radar WebKit Bug Importer 2020-05-31 16:18:45 PDT
<rdar://problem/63812487>
Comment 2 Mark Lam 2020-05-31 17:41:49 PDT
Created attachment 400710 [details]
proposed patch.
Comment 3 Mark Lam 2020-05-31 17:54:09 PDT
Created attachment 400711 [details]
proposed patch.
Comment 4 Mark Lam 2020-05-31 18:05:01 PDT
Created attachment 400712 [details]
proposed patch.
Comment 5 Mark Lam 2020-05-31 18:29:33 PDT
Created attachment 400713 [details]
work in progress for EWS testing.
Comment 6 Mark Lam 2020-05-31 18:37:40 PDT
Created attachment 400714 [details]
work in progress for EWS testing.
Comment 7 Mark Lam 2020-05-31 18:40:33 PDT
Created attachment 400715 [details]
work in progress for EWS testing.
Comment 8 Mark Lam 2020-05-31 21:41:19 PDT
Created attachment 400720 [details]
proposed patch.
Comment 9 Mark Lam 2020-05-31 22:10:26 PDT
Created attachment 400721 [details]
proposed patch.
Comment 10 Mark Lam 2020-05-31 23:00:35 PDT
Looks like Win EWS bot failure is unrelated to my patch.  That means this is ready for a review.
Comment 11 Yusuke Suzuki 2020-06-01 09:25:43 PDT
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.
Comment 12 Mark Lam 2020-06-01 10:15:33 PDT
*** Bug 212599 has been marked as a duplicate of this bug. ***
Comment 13 Mark Lam 2020-06-02 00:57:48 PDT
(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.
Comment 14 Mark Lam 2020-06-02 00:58:21 PDT
Created attachment 400792 [details]
proposed patch.
Comment 15 Guillaume Emont 2020-06-02 06:19:05 PDT
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.
Comment 16 Mark Lam 2020-06-02 06:43:32 PDT
Created attachment 400813 [details]
propose patch.
Comment 17 Mark Lam 2020-06-02 07:18:44 PDT
Comment on attachment 400813 [details]
propose patch.

I think this is ready for a review now.
Comment 18 Yusuke Suzuki 2020-06-02 12:19:29 PDT
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/
Comment 19 Mark Lam 2020-06-02 12:45:45 PDT
Thanks for the review.  Landed in r262434: <http://trac.webkit.org/r262434>.