Closed Bug 1245294 Opened 4 years ago Closed 2 years ago

VanillaJS-TodoMVC, jQuery-TodoMVC, FlightJS-TodoMVC of Speedometer test seems to spent significant amount time in or under parser

Categories

(Core :: HTML: Parser, defect)

36 Branch
defect
Not set

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
platform-rel --- -

People

(Reporter: smaug, Unassigned)

References

(Blocks 1 open bug)

Details

See bug 1245279.

I haven't yet analyzed too much where the time is spent, but
I assume parser could use some inlining and then DOM handling, especially attribute setting could be optimized (in a different bug blocking this one).
nsHtml5Tokenizer::emitCurrentTagToken is a case perhaps worth to inline.
Not too large method and called very often.
Also nsHtml5ElementName::elementNameByBuffer
Whiteboard: dom-noted
Whiteboard: dom-noted → dom-triaged
Whiteboard: dom-triaged → dom-triaged [platform-rel-jQuery]
platform-rel: --- → ?
platform-rel: ? → -
Whiteboard: dom-triaged [platform-rel-jQuery]
Flags: needinfo?(hsivonen)
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #1)
> nsHtml5Tokenizer::emitCurrentTagToken is a case perhaps worth to inline.

The reason why this isn't inlined is that the method calls methods on tokenHandler (nsHtml5TreeBuilder). nsHtml5TreeBuilder is forward-declared only in nsHtml5Tokenizer.h to avoid a circular dependency between the two header files.

> Not too large method and called very often.
> Also nsHtml5ElementName::elementNameByBuffer

The reason why this isn't inlined is that the method instantiates a subclass of nsHtml5ElementName and the header for the subclass can't be can't be included before the the name of the subclass is referenced, because by that time the declaration of the superclass isn't yet complete.

smaug, do you have suggestions on how to proceed? Manually inlining these (as opposed to declaring them 'inline') would be bad for maintainability.
Flags: needinfo?(hsivonen) → needinfo?(bugs)
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> > Not too large method and called very often.
> > Also nsHtml5ElementName::elementNameByBuffer
> 
> The reason why this isn't inlined is that the method instantiates a subclass
> of nsHtml5ElementName and the header for the subclass can't be can't be
> included before the the name of the subclass is referenced, because by that
> time the declaration of the superclass isn't yet complete.

This could be solved by distinguishing releasable and non-releasable objects using a boolean flag instead of using a vtable. I.e. by getting rid of nsHtml5ReleasableElementName as a distinct class.
I think profiling this again and looking at what the profile says could be useful. There seems to be many small things in parser to possibly optimize.
Flags: needinfo?(bugs)
Given recent work you've done here, Henri, is this still valid?
Flags: needinfo?(hsivonen)
We have still related parser bugs open. 
Bug 1351857, Bug 1355441 and bug 1355479 at least. 
And I had forgotten the idea to inline some methods.
(In reply to Andrew Overholt [:overholt] from comment #5)
> Given recent work you've done here, Henri, is this still valid?

I think that's more of a question for smaug, since he can repeat the previous profiling.

However, a patch that landed earlier this week seems to have (at least on surface) removed the reason not to inline nsHtml5ElementName::elementNameByBuffer that I stated in comment 2, so I guess I should try inlining it now.
Flags: needinfo?(hsivonen)
Depends on: 1358037
If this bug is still valid for V2 it should likely be qf:p1
Flags: needinfo?(bugs)
Whiteboard: [qf]
We have tracked parser related issues in other, newer bugs.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(bugs)
Resolution: --- → INCOMPLETE
Whiteboard: [qf]
No longer blocks: TimeToFirstPaint_FB
You need to log in before you can comment on or make changes to this bug.