Bug 231283 – [Build-time perf] Forward-declare more things in Element.h

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Build-time perf] Forward-declare more things in Element.h

[Jer Noble]

Created attachment 440341 [details]
Patch

[Jer Noble]

Created attachment 440343 [details]
Patch

[Jer Noble]

Created attachment 440345 [details]
Patch

[Jer Noble]

Created attachment 440377 [details]
Patch

[Jer Noble]

Created attachment 440379 [details]
Patch

[Jer Noble]

Created attachment 440401 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Darin Adler]

Comment on attachment 440401 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440401&action=review

Can we measure a build speed-up? I love these types of changes, and want to make sure we do them based on evidence.

> Source/WebCore/ChangeLog:9
> +        Replace as many #includes as possible with forward type declarations. To do so, split out the inline function
> +        definitions into their own ElementInlines.h header.

How do we know when to add includes of ElementInines.h?

Do we get compilation failures when we forget them?

> Source/JavaScriptCore/heap/HandleForward.h:32
> +template <class T> class Handle;

Better syntax:

    template<typename> class Handle;

No need to include a name if it’s just "T".

> Source/JavaScriptCore/heap/StrongForward.h:32
> +template <typename T, ShouldStrongDestructorGrabLock = ShouldStrongDestructorGrabLock::No> class Strong;

I prefer this syntax:

    template<typename, ShouldStrongDestructorGrabLock = ShouldStrongDestructorGrabLock::No> class Strong;

> Source/WebCore/html/HTMLMediaElement.h:588
> +    void privateBrowsingStateDidChange(const PAL::SessionID&);

This is an unfortunate change. Since a session ID is just a scalar, we would like to pass by value. The change to use const& should not be necessary. We should be able to compile a declaration without the class being defined, although not calls to the function. Did that not work?

> Source/WebCore/html/HTMLMediaElement.h:953
> +    void setControllerJSProperty(const char*, JSC::JSValue&&);

This change doesn’t makes sense. JSValue is a scalar, and it doesn’t make sense to pass it by rvalue reference.

[Jer Noble]

Created attachment 440415 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 440415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440415&action=review

> Source/WebCore/ChangeLog:22
> +        With these changes in place, the net cost of Element.h during a typical build has gone from ~750s to ~57s.

Nice!

[Jer Noble]

Created attachment 440417 [details]
Patch

[Darin Adler]

Comment on attachment 440417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440417&action=review

> Source/WebCore/dom/EventOptions.h:33
> +enum class EventIsTrusted : uint8_t { No, Yes };
> +enum class EventCanBubble : uint8_t { No, Yes };
> +enum class EventIsCancelable : uint8_t { No, Yes };
> +enum class EventIsComposed : uint8_t { No, Yes };

How about bool instead of uint8_t for these?

[Jer Noble]

Created attachment 440432 [details]
Patch

[Jer Noble]

(Sorry Darin, not ignoring your comments; just trying to fix all the build errors first.)

[Darin Adler]

(In reply to Jer Noble from comment #14)
> (Sorry Darin, not ignoring your comments; just trying to fix all the build
> errors first.)

No rush.

[Jer Noble]

(In reply to Darin Adler from comment #8)
> Comment on attachment 440401 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440401&action=review
> 
> Can we measure a build speed-up? I love these types of changes, and want to
> make sure we do them based on evidence.

Clang recently added support for doing time-based profiling. Passing in the -ftime-trace flag to clang will generate .json files containing time information on a per-source-file, per-header, per-function, and per-template basis. I'm using a utility called ClangBuildAnalyzer <https://github.com/aras-p/ClangBuildAnalyzer> to pull all the .json files together for net timing information.

Before these changes, that tool reported the following for Element.h:

*** Expensive headers:
754617 ms: dom/Element.h (included 285 times, avg 2647 ms), included via:
  AttributeChangeInvalidation.cpp AttributeChangeInvalidation.h  (6086 ms)
  RenderTreeUpdater.cpp RenderTreeUpdater.h RenderTreeBuilder.h RenderTreePosition.h RenderElement.h RenderObject.h  (5736 ms)
  RenderTreeBuilderInline.cpp RenderTreeBuilderInline.h RenderTreeBuilder.h RenderTreePosition.h RenderElement.h RenderObject.h  (5583 ms)
  PropertyCascade.cpp PropertyCascade.h ElementRuleCollector.h RuleSet.h RuleData.h SelectorFilter.h  (5129 ms)
  SVGTextQuery.cpp LegacyInlineFlowBox.h LegacyInlineBox.h RenderBoxModelObject.h RenderLayerModelObject.h RenderElement.h RenderObject.h  (4900 ms)
  StyleResolver.cpp StyleResolver.h ElementRuleCollector.h RuleSet.h RuleData.h SelectorFilter.h  (4847 ms)

Afterward:

57889 ms: dom/Element.h (included 287 times, avg 201 ms), included via:
  SelectorCompiler.cpp SelectorCompiler.h SelectorChecker.h  (1171 ms)
  VisitedLinkState.cpp VisitedLinkState.h  (1029 ms)
  AttributeChangeInvalidation.cpp AttributeChangeInvalidation.h  (1013 ms)
  CachedScriptFetcher.cpp CachedResourceLoader.h CachedResourceRequest.h  (1001 ms)
  JSHTMLOptionsCollection.cpp JSHTMLOptionsCollection.h HTMLOptionsCollection.h CachedHTMLCollection.h HTMLCollection.h LiveNodeList.h CollectionTraversal.h ElementIterator.h ElementTraversal.h  (949 ms)
  PseudoElement.cpp PseudoElement.h  (945 ms)
  ...

> > Source/WebCore/ChangeLog:9
> > +        Replace as many #includes as possible with forward type declarations. To do so, split out the inline function
> > +        definitions into their own ElementInlines.h header.
> 
> How do we know when to add includes of ElementInines.h?
> 
> Do we get compilation failures when we forget them?

Unfortunately no; if you don't include ElementInlines.h you'll get a linker error, not a compile error. I'm searching for a mechanism to annotate these function declarations to generate a compiler error if you don't also include ElementInlines.h, but I haven't found it yet.

> > Source/JavaScriptCore/heap/HandleForward.h:32
> > +template <class T> class Handle;
> 
> Better syntax:
> 
>     template<typename> class Handle;
> 
> No need to include a name if it’s just "T".

Ok.

> > Source/JavaScriptCore/heap/StrongForward.h:32
> > +template <typename T, ShouldStrongDestructorGrabLock = ShouldStrongDestructorGrabLock::No> class Strong;
> 
> I prefer this syntax:
> 
>     template<typename, ShouldStrongDestructorGrabLock =
> ShouldStrongDestructorGrabLock::No> class Strong;

Ok.

> > Source/WebCore/html/HTMLMediaElement.h:588
> > +    void privateBrowsingStateDidChange(const PAL::SessionID&);
> 
> This is an unfortunate change. Since a session ID is just a scalar, we would
> like to pass by value. The change to use const& should not be necessary. We
> should be able to compile a declaration without the class being defined,
> although not calls to the function. Did that not work?

Let me try that; I was under the impression that all function parameters had to be fully defined (but not function return types), but I absolutely could be mistaken.

> > Source/WebCore/html/HTMLMediaElement.h:953
> > +    void setControllerJSProperty(const char*, JSC::JSValue&&);
> 
> This change doesn’t makes sense. JSValue is a scalar, and it doesn’t make
> sense to pass it by rvalue reference.

Same here. Will try.

[Darin Adler]

(In reply to Jer Noble from comment #16)
> I'm using a utility called ClangBuildAnalyzer
> <https://github.com/aras-p/ClangBuildAnalyzer> to pull all the .json files
> together for net timing information.

One of the WebKit team best practices has been to put these kinds of analyses up where people can see them, leads to competition and teamwork to fix the problems. For example, long ago Kling put up a list of the largest functions, and we cut them down to size.

I think you should consider putting this somewhere people can see it so they don’t need to learn to run the tool.

[Darin Adler]

(In reply to Jer Noble from comment #16)
> Unfortunately no; if you don't include ElementInlines.h you'll get a linker
> error, not a compile error.

Linker error in release builds only? Or linker error even in debug builds?

[Jer Noble]

(In reply to Jer Noble from comment #16)
> Unfortunately no; if you don't include ElementInlines.h you'll get a linker
> error, not a compile error. I'm searching for a mechanism to annotate these
> function declarations to generate a compiler error if you don't also include
> ElementInlines.h, but I haven't found it yet.

Oh duh. I was searching for things like __attribute__(always_inline), but the answer is much simpler:

class Foo {
public:
    inline void bar();
};

void test(Foo foo)
{
    foo.bar();
}

<source>:3:17: warning: inline function 'Foo::bar' is not defined [-Wundefined-inline]
    inline void bar();
                ^
<source>:8:9: note: used here
    foo.bar();
        ^
1 warning generated.

We just need to decorate all the inlined functions in ElementInlines.h with `inline` in Element.h.

[Darin Adler]

(In reply to Jer Noble from comment #19)
> We just need to decorate all the inlined functions in ElementInlines.h with
> `inline` in Element.h.

Seems like it would also be a big win to do this in the many places in JavaScriptCore that use this same idiom. And for functions where we define them in one header, but implement in another (Node.h vs. Element.h). This may not work quite as well for templates, but it sure does seem like it will help us a lot!

[Jer Noble]

(In reply to Darin Adler from comment #17)
> (In reply to Jer Noble from comment #16)
> > I'm using a utility called ClangBuildAnalyzer
> > <https://github.com/aras-p/ClangBuildAnalyzer> to pull all the .json files
> > together for net timing information.
> 
> One of the WebKit team best practices has been to put these kinds of
> analyses up where people can see them, leads to competition and teamwork to
> fix the problems. For example, long ago Kling put up a list of the largest
> functions, and we cut them down to size.
> 
> I think you should consider putting this somewhere people can see it so they
> don’t need to learn to run the tool.

That's a great idea. I'll put together a Wiki.

[Jer Noble]

(In reply to Jer Noble from comment #16)
> Let me try that; I was under the impression that all function parameters had
> to be fully defined (but not function return types), but I absolutely could
> be mistaken.

And indeed I was mistaken; reverting that change (to just pass in a scalar) works just fine with a forward-definition. Will revert the other as well.

[Jer Noble]

Decorating with `inline` revealed this fun warning/error:

Source/WebCore/dom/Element.h:130:32: error: inline function 'WebCore::Element::hasAttributes' is not defined [-Werror,-Wundefined-inline]
    inline WEBCORE_EXPORT bool hasAttributes() const;
                               ^
In file included from WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource5-mm.mm:7:
In file included from ./accessibility/mac/AccessibilityObjectMac.mm:30:
In file included from Source/WebCore/dom/ElementAncestorIterator.h:28:
In file included from Source/WebCore/dom/ElementIterator.h:28:
Source/WebCore/dom/ElementInlines.h:54:59: note: used here
    return is<Element>(*this) && downcast<Element>(*this).hasAttributes();

Looks like (A) this decoration works and (B) I have to make sure hasAttributes() appears first in ElementInlines.h.

[Jer Noble]

(In reply to Jer Noble from comment #23)
> Looks like (A) this decoration works and (B) I have to make sure
> hasAttributes() appears first in ElementInlines.h.

Disregard. (A) works but I mistakenly decorated Element::hasAttributes() instead of Node::hasAttributes().

[Jer Noble]

Created attachment 440442 [details]
Patch

[Jer Noble]

Created attachment 440467 [details]
Patch

[Jer Noble]

Created attachment 440540 [details]
Patch

[Jer Noble]

Created attachment 440548 [details]
Patch

[Jer Noble]

Created attachment 440549 [details]
Patch

[Jer Noble]

FYI: here's the Wiki which I've been updating as I go: <https://trac.webkit.org/wiki/AnalyzingBuildPerformance>

[Jer Noble]

Created attachment 440630 [details]
Patch

[Jer Noble]

Created attachment 440635 [details]
Patch for landing

[EWS]

Committed r283851 (242730@main): <https://commits.webkit.org/242730@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440635 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84048751>