Closed Bug 1011826 Opened 10 years ago Closed 8 years ago

Consider reducing the number of function pointers stored in Prefable

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

We should be able to combine the AvailableIn and CheckPermissions bits into a single function that gets passed the permission string and an enum for where we're available or something.

Peter, thoughts?  Not worth it?  Mostly I want to make the common case when we don't have any of this stuff in the Prefable be fast...
Flags: needinfo?(peterv)
Fine by me.
Flags: needinfo?(peterv)
Blocks: 1048695
Flags: needinfo?(bzbarsky)
And now we have two different permission strings.

> Mostly I want to make the common case when we don't have any of this stuff in the
> Prefable be fast...

Maybe we should just have a flat-out "has any conditions" boolean, which if false means we're enabled, period.
Attached patch Split Prefable into two pieces (obsolete) — Splinter Review
Here's an example of the new output:

> static const PrefableDisablers sAttributes_disablers74 = {
>   0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
> static const PrefableDisablers sAttributes_disablers77 = {
>   0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
> static const PrefableDisablers sAttributes_disablers79 = {
>   0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
> static const PrefableDisablers sAttributes_disablers81 = {
>   0, &TestFuncControlledMember, nullptr, nullptr, nullptr
> };
> static const PrefableDisablers sAttributes_disablers84 = {
>   0, &TestFuncControlledMember, nullptr, nullptr, nullptr
> };
> 
> // Can't be const because the pref-enabled boolean needs to be writable
> static Prefable<const JSPropertySpec> sAttributes[] = {
>   { true, nullptr, &sAttributes_specs[0] },
>   { true, nullptr, &sAttributes_specs[66] },
>   { true, nullptr, &sAttributes_specs[69] },
>   { true, nullptr, &sAttributes_specs[72] },
>   { true, &sAttributes_disablers74, &sAttributes_specs[74] },
>   { true, &sAttributes_disablers77, &sAttributes_specs[77] },
>   { true, &sAttributes_disablers79, &sAttributes_specs[79] },
>   { true, &sAttributes_disablers81, &sAttributes_specs[81] },
>   { true, &sAttributes_disablers84, &sAttributes_specs[84] },
>   { true, nullptr, &sAttributes_specs[87] },
>   { false, nullptr, nullptr }
> };

This case is one where we have duplicate PrefableDisablers structs, but that's
rare in practice and I don't think it's worth trying to avoid.
Attachment #8733202 - Flags: review?(bzbarsky)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Comment on attachment 8733202 [details] [diff] [review]
Split Prefable into two pieces

>+        return self.func != "nullptr" or \

This file prefers parens around stuff like this instead of using \ to escape newlines.

>         # So we won't put a specTerminator at the very front of the list:

This comment needs to stay with the line it's commenting, which is:

         lastCondition = getCondition(array[0], self.descriptor)

>+        disablersTemplate1 = 'static const PrefableDisablers %s_disablers%d = {'

Why not just have a single string as the disablersTemplate, including the trailing "};" and whatnot, so it's clearer how the pieces fit together?  Also, it would probably be a good idea to include a \n after the "};" so the disablers aren't as bunched up.

>+        def switchToCondition1(props, condition):

Why do we need to do two loops over the members?  What goes wrong if we just do it in one loop?  I'm not seeing anything obvious, but maybe it's subtle?  If so, it definitely needs at least documenting...  Then you would not need to maintain specsLen either, right?

>+                # And switch to our new pref

New condition.

>+struct PrefableDisablers {

Is there a reason you didn't move the "enabled" boolean into here as well?  Seems like it should be quite doable.  I guess it's not clear whether the tradeoff is necessarily worth it; worth looking at CSS2PropertiesBinding.cpp as the "hard case" here.

If this is _not_ worth it, please document why not.  And document the hasDisablers() function, since it in fact ignores the preference (if any) and only considers the other ways to disable the API.

>+    // exposure set (within PrefableCondition) before checking "enabled" we

s/PrefableCondition/PrefableDisablers/, right?

I'd like to see the patch updated to the above before marking r+.
Attachment #8733202 - Flags: review?(bzbarsky)
And thank you for doing it!
> >+        def switchToCondition1(props, condition):
> 
> Why do we need to do two loops over the members?  What goes wrong if we just
> do it in one loop?

I was going to say "because we need to print all the disablers before the specs so they aren't intermingled", but of course we don't actually print them in the loop, just gather them up. Duh :) I will combine the loops and that'll make it a lot neater.

I'll address the other changes too. Thank you for the fast review.
> >+struct PrefableDisablers {
> 
> Is there a reason you didn't move the "enabled" boolean into here as well? 
> Seems like it should be quite doable.  I guess it's not clear whether the
> tradeoff is necessarily worth it; worth looking at CSS2PropertiesBinding.cpp
> as the "hard case" here.

|enabled| can change via Preferences::AddBoolVarCache(), right? If a Prefable lacked a PrefableDisablers then |enabled| would be not be explicitly present, which presumably would imply a value of |true|. But if the pref was flipped, we'd need to explicit record a |false| value, which would require creating a new PrefableDisablers at runtime.

It certainly would be nice to remove |enabled| from Prefable, because it's a single bit of information that takes up a whole word. I suppose we could do something hacky like this:

- |conds| == 0 --> enabled
- |conds| == 1 --> disabled
- |conds| == <pointer> --> consult the PrefableDisabler

(Or vice versa on the first two.) Thoughts?
Flags: needinfo?(bzbarsky)
> I suppose we could do something hacky like this:
> 
> - |conds| == 0 --> enabled
> - |conds| == 1 --> disabled
> - |conds| == <pointer> --> consult the PrefableDisabler

A problem case: assume |conds| initially has a <pointer> value. Then we change the pref to disable it. Then we change the pref back to enable it -- in that case the <pointer> value should be restored, but it's been lost and would be hard to recover.
(In the above two comments where I wrote |conds| I should have written |disablers|. Sorry for any confusion.)

I guess we could just use a tagged pointer for |disablers|. AddBoolVarCache() would need to be changed accordingly, but that's doable.
> Is there a reason you didn't move the "enabled" boolean into here as well? 
> Seems like it should be quite doable.  I guess it's not clear whether the
> tradeoff is necessarily worth it; worth looking at CSS2PropertiesBinding.cpp
> as the "hard case" here.

I measured and it is worth it. There are 1559 cases that have no disablers and lack a pref. There are only 153 cases that have no disablers but have a pref. So by doing this we remove 1559 words on the former cases and add 5 words for the latter cases. The end result is 1559 - 5*153 = 794 words removed.
Flags: needinfo?(bzbarsky)
This version is much nicer, and increases the static data saving from 69,552
bytes to 92,820 bytes on my Linux64 box.

I changed |nonExposedGlobals| from uint32_t to uint16_t. This reduces
sizeof(PrefableDisablers) by one word on 32-bit because it lets
|nonExposedGlobals| be packed into the same word as |enabled|.

Moving |enabled| into PrefableDisablers does introduce one possibility for
error that wasn't there previously -- if something goes wrong with codegen we
could try to call AddBoolVarCache() with &blah.disablers->enabled when
blah.disablers is null. But only if Codegen.py is buggy.

Here is the new version of the code from comment 3. The first three disablers
are new -- they are cases that have a pref but no other form of disabling.

> static PrefableDisablers sAttributes_disablers66 = {
>   true, 0, nullptr, nullptr, nullptr, nullptr
> };
> 
> static PrefableDisablers sAttributes_disablers69 = {
>   true, 0, nullptr, nullptr, nullptr, nullptr
> };
> 
> static PrefableDisablers sAttributes_disablers72 = {
>   true, 0, nullptr, nullptr, nullptr, nullptr
> };
>   
> static PrefableDisablers sAttributes_disablers74 = {
>   true, 0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
>   
> static PrefableDisablers sAttributes_disablers77 = {
>   true, 0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
> 
> static PrefableDisablers sAttributes_disablers79 = {
>   true, 0, &nsGenericHTMLElement::TouchEventsEnabled, nullptr, nullptr, nullptr
> };
>   
> static PrefableDisablers sAttributes_disablers81 = {
>   true, 0, &TestFuncControlledMember, nullptr, nullptr, nullptr
> };
> 
> static PrefableDisablers sAttributes_disablers84 = {
>   true, 0, &TestFuncControlledMember, nullptr, nullptr, nullptr
> };
> 
> // Can't be const because the pref-enabled boolean needs to be writable
> static Prefable<const JSPropertySpec> sAttributes[] = {
>   { nullptr, &sAttributes_specs[0] },
>   { &sAttributes_disablers66, &sAttributes_specs[66] },
>   { &sAttributes_disablers69, &sAttributes_specs[69] },
>   { &sAttributes_disablers72, &sAttributes_specs[72] },
>   { &sAttributes_disablers74, &sAttributes_specs[74] },
>   { &sAttributes_disablers77, &sAttributes_specs[77] },
>   { &sAttributes_disablers79, &sAttributes_specs[79] },
>   { &sAttributes_disablers81, &sAttributes_specs[81] },
>   { &sAttributes_disablers84, &sAttributes_specs[84] },
>   { nullptr, &sAttributes_specs[87] },
>   { nullptr, nullptr }
> };
Attachment #8733650 - Flags: review?(bzbarsky)
Attachment #8733202 - Attachment is obsolete: true
Blocks: 1254777
Comment on attachment 8733650 [details] [diff] [review]
Split Prefable into two pieces

>+        disablersTemplate = \

This would probably be more readable if you just used dedent() and a string inside """.

>+  // because it will changed at runtime if the corresponding pref is changed.

s/will changed/will change/

r=me.  This is awesome.  :)
Attachment #8733650 - Flags: review?(bzbarsky) → review+
Whiteboard: [MemShrink]
(In reply to Boris Zbarsky [:bz] from comment #13)
> 
> r=me.  This is awesome.  :)

It turned out well! Thank you for all your help.
> >+        disablersTemplate = \
> 
> This would probably be more readable if you just used dedent() and a string
> inside """.

I tried a """ string but changed it because of the indentation. I didn't know about dedent(), that's useful! Thank you.
Whiteboard: [MemShrink] → [MemShrink:P2]
https://hg.mozilla.org/mozilla-central/rev/531508a2e755
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.