Closed
Bug 1011826
Opened 10 years ago
Closed 9 years ago
Consider reducing the number of function pointers stored in Prefable
Categories
(Core :: DOM: Core & HTML, defect)
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)
10.16 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 4•9 years ago
|
||
Reporter | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
And thank you for doing it!
Assignee | ||
Comment 7•9 years ago
|
||
> >+ 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.
Assignee | ||
Comment 8•9 years ago
|
||
> >+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)
Assignee | ||
Comment 9•9 years ago
|
||
> 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.
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
> 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)
Assignee | ||
Comment 12•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8733202 -
Attachment is obsolete: true
Reporter | ||
Comment 13•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #13)
>
> r=me. This is awesome. :)
It turned out well! Thank you for all your help.
Assignee | ||
Comment 15•9 years ago
|
||
> >+ 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.
Updated•9 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/531508a2e75575e007d3eb578c77fd3ecf5fee1e
Bug 1011826 - Split Prefable into two pieces. r=bz.
Comment 17•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•