Closed Bug 1421544 Opened 7 years ago Closed 7 years ago

Lazy push/pop CustomElementReactionsStack entry for [CEReactions] operations

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(1 file)

Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;
59 bytes, text/x-review-board-request
smaug
: review+
Details 
(In reply to Edgar Chen [:edgar] from comment #1)
> Created attachment 8932770 [details]
> Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;
> 
> Review commit: https://reviewboard.mozilla.org/r/203804/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/203804/

Test: https://bugzilla.mozilla.org/attachment.cgi?id=8932732

Base rev: 3f5d48c08903

Without patch:
 - pref off: average time is about 112 ms
 - pref on:  average time is about 139 ms

With patch:
 - pref off: average time is about 109 ms
 - pref on:  average time is about 116 ms
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

I'd like to ask for earlier feedback for this approach.
Attachment #8932770 - Flags: feedback?(bugs)
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review209294

::: dom/base/CustomElementRegistry.h:328
(Diff revision 1)
> +        ases.emplace(aCx);
> +      }
> +      PopAndInvokeElementQueue();
> +    }
> +    mLastElementRecursionDepth = aRestoredRecursionDepth;
> +    mRecursionDepth--;

Could you explain why we want
mLastElementRecursionDepth = aRestoredRecursionDepth;
mRecursionDepth--;
after calling PopAndInvokeElementQueue() and not before?
Attachment #8932770 - Flags: feedback?(bugs) → feedback?(echen)
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review209294

> Could you explain why we want
> mLastElementRecursionDepth = aRestoredRecursionDepth;
> mRecursionDepth--;
> after calling PopAndInvokeElementQueue() and not before?

We don't need 
```
mLastElementRecursionDepth = aRestoredRecursionDepth;
mRecursionDepth--;
```
before is because we pushed element queue to the stack for every CEReaction recursion and popped whenever leaving the recursion.

But now we might not push element queue to the stack in a recursion if it doesn't try to enqueue reactions and we don't need to process the stack when leaving the recursion, either.

Consider the case that there are 3 level recursions, the first and thrid level will create and push element queue to the stack, but second one won't,

[CEReaction_1] Operation_1
   (create and push element queue to stack)
   [CEReaction_2] Operation_2
      (do nothing special)
      [CEReaction_3] Operation_3
         (create and push element queue to stack)
         ...
         (pop and invoke reactions)
      (do nothing special)
   (pop and invoke reactions)

When we leave the thrid level, we have to invoke reactions and pop the top entry of the stack.
But we don't need to do anything for the leaving of second level.
So we kinda need a flag to keep status of each recursion and I use a bit tricky way.

Basically, mStoredRecursionDepth in AutoCEReaction is used for keeping "previous" level's status to restore the status when we go back to the level, so that we can know whether we need to process the stack when leaving the level.

The detail steps is like,

{mRecursionDepth=0, mLastElementRecursionDepth=0}
[CEReaction_1] Operation_1
   {AutoCEReaction_1::mStoredRecursionDepth=0}
   {mRecursionDepth=1, mLastElementRecursionDepth=0}
   (create and push element queue to stack) <----- update mLastElementRecursionDepth
   {mRecursionDepth=1, mLastElementRecursionDepth=1}
   [CEReaction_2] Operation_2
      {AutoCEReaction_2::mStoredRecursionDepth=1}
      {mRecursionDepth=2, mLastElementRecursionDepth=1}
      (do nothing special)
      [CEReaction_3] Operation_3
         {AutoCEReaction_3::mStoredRecursionDepth=1}
         {mRecursionDepth=3, mLastElementRecursionDepth=1}
         (create and push element queue to stack) <----- update mLastElementRecursionDepth
         {mRecursionDepth=3, mLastElementRecursionDepth=3>
         ...
         (pop and invoke reactions) <----- becasue of mRecursionDepth == mLastElementRecursionDepth
      {mRecursionDepth=2, mLastElementRecursionDepth=1} <----- restore the mLastElementRecursionDepth
      (do nothing special) <----- because of mRecursionDepth != mLastElementRecursionDepth
   {mRecursionDepth=1, mLastElementRecursionDepth=1} <----- restore the mLastElementRecursionDepth
   (pop and invoke reactions) <----- becasue of mRecursionDepth == mLastElementRecursionDepth
{mRecursionDepth=0, mLastElementRecursionDepth=0} <----- restore the mLastElementRecursionDepth
Attachment #8932770 - Flags: feedback?(echen) → feedback?(bugs)
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review210768

::: dom/base/CustomElementRegistry.h:373
(Diff revision 1)
>    void Enqueue(Element* aElement, CustomElementReaction* aReaction);
>  
> +  // Current [CEReactions] recursion depth.
> +  uint32_t mRecursionDepth;
> +  // Recursion depth of the top entry of reaction stack.
> +  uint32_t mLastElementRecursionDepth;

I think this needs better comment explaining how this differs from mRecursionDepth.
Or could this be even renamed. It after all doesn't tell element recursion depth, but what was the recursion depth when something was pushed to reaction stack.
mRecursionDepthOnLastElementQueuePush?
I know a bit long, but would that be a bit more descriptive, perhaps.

::: dom/base/CustomElementRegistry.h:561
(Diff revision 1)
>      }
> +
>    private:
>      RefPtr<CustomElementReactionsStack> mReactionsStack;
>      JSContext* mCx;
> +    uint32_t mStoredRecursionDepth;

What is mStoredRecursionDepth?
Apparently it is a copy of mLastElementRecursionDepth and not mRecursionDepth as one would expect from the name
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

I think the approach is reasonable. A bit hard to follow (which is why good comments and good variable names are needed), but optimizes out some slow operations.
Attachment #8932770 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review210768

(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #8)
> A bit hard to follow (which is why good comments and good variable names are needed)

Yeah, totally agree, I will try to add more comments and come out a better variable names. Thank you!!

> I think this needs better comment explaining how this differs from mRecursionDepth.
> Or could this be even renamed. It after all doesn't tell element recursion depth, but what was the recursion depth when something was pushed to reaction stack.
> mRecursionDepthOnLastElementQueuePush?
> I know a bit long, but would that be a bit more descriptive, perhaps.

Will do.

> What is mStoredRecursionDepth?
> Apparently it is a copy of mLastElementRecursionDepth and not mRecursionDepth as one would expect from the name

Yes, it is a copy of mLastElementRecursionDepth, will come out a better name to avoid confusion and perhaps add more comments.
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203802/#review211374

(In reply to Edgar Chen [:edgar] from comment #9)
> I think this needs better comment explaining how this differs from mRecursionDepth.
> Or could this be even renamed. It after all doesn't tell element recursion depth, but what was the recursion depth when something was pushed to reaction stack.
> mRecursionDepthOnLastElementQueuePush?
> I know a bit long, but would that be a bit more descriptive, perhaps.

In this version, I change to use bool type and rename it to mIsElementQueuePushedForCurrentRecursionDepth. The main idea is the same, but it becomes more easier to follow, I would think. I am happy to add more comments if you think there is something we need to explain more. Thank you.
Attachment #8932770 - Flags: review?(bugs)
Sorry, didn't get to this today. I'll try tomorrow.
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review212490

I like mIsElementQueuePushedForCurrentRecursionDepth. Makes this easier to understand.

::: dom/base/CustomElementRegistry.h:288
(Diff revision 3)
> +      PopAndInvokeElementQueue();
> +    }
> +    mRecursionDepth--;
> +    // Restore the is-element-queue-pushed flag cached in AutoCEReaction when
> +    // leaving the recursion level.
> +    mIsElementQueuePushedForCurrentRecursionDepth = aIsElementQueuePushed;

This is still very confusing. 
We use mIsElementQueuePushedForCurrentRecursionDepth earlier in the method and then set it to aIsElementQueuePushed.
Could aIsElementQueuePushed be called
aWasElementQueuePushed
Attachment #8932770 - Flags: review?(bugs) → review+
Comment on attachment 8932770 [details]
Bug 1421544 - Lazy push/pop CustomElementReactionsStack entry;

https://reviewboard.mozilla.org/r/203804/#review212668

::: dom/base/CustomElementRegistry.h:288
(Diff revision 3)
> +      PopAndInvokeElementQueue();
> +    }
> +    mRecursionDepth--;
> +    // Restore the is-element-queue-pushed flag cached in AutoCEReaction when
> +    // leaving the recursion level.
> +    mIsElementQueuePushedForCurrentRecursionDepth = aIsElementQueuePushed;

Yeah, aWasElementQueuePushed is much better.
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb5613b3d40b
Lazy push/pop CustomElementReactionsStack entry; r=smaug
https://hg.mozilla.org/mozilla-central/rev/cb5613b3d40b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: