1195838 - Maintain all the TimelineMarker subclasses in a single place

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

Comment 2

[Tom Tromey :tromey]

Comment on attachment 8649349 [details] [diff] [review]
v1

Review of attachment 8649349 [details] [diff] [review]:
-----------------------------------------------------------------

As a meta note, I find it useful to try to make moves "pure", then make other code changes
as a separate patch; or at least make a list of the simultaneous changes.  This makes it
simpler to review.

Overall I think this patch is fine, though I don't know why it is desirable.  Some justification
would be good; without it I tend to think that it's maybe better to keep the timeline subclasses
near the code that is manipulating them.

I have no idea whether or not you need reviews from peers for all the affected modules.

::: docshell/base/nsDocShell.h
@@ +38,2 @@
>  #include "timeline/TimelineMarker.h"
> +#include "timeline/JavascriptTimelineMarker.h"

Is this include really needed in the .h?  It seems like the only reference to the class is in the .cpp, so the include could go there instead.

::: docshell/base/timeline/LayerTimelineMarker.h
@@ +22,5 @@
> +  ~LayerTimelineMarker()
> +  {}
> +
> +  virtual void AddLayerRectangles(dom::Sequence<dom::ProfileTimelineLayerRect>& aRectangles)
> +  {

I don't think this has to be virtual any more; though see the other comment.

Also, I'm not sure if Mozilla tries to follow "include what you use", but if so, then this header needs a bunch more includes.  Perhaps that's not super relevant in the unified build world.

::: docshell/base/timeline/ObservedDocShell.cpp
@@ +80,5 @@
>          bool endIsLayerType = strcmp(endPayload->GetName(), "Layer") == 0;
>  
>          // Look for "Layer" markers to stream out "Paint" markers.
>          if (startIsPaintType && endIsLayerType) {
> +          LayerTimelineMarker* layerPayload = static_cast<LayerTimelineMarker*>(endPayload.get());

I think this is less safe than the previous approach.  Now, if someone makes a coding mistake somewhere, some object will be cast to the wrong type, with incorrect but also indeterminate results.  With the previous approach, you'd be guaranteed a crash.

Comment 3

[Victor Porof [:vporof][:vp]]

(In reply to Tom Tromey :tromey from comment #2)
> Comment on attachment 8649349 [details] [diff] [review]
> v1
> 
> Review of attachment 8649349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As a meta note, I find it useful to try to make moves "pure", then make
> other code changes
> as a separate patch; or at least make a list of the simultaneous changes. 
> This makes it
> simpler to review.
> 

Agreed. Although the changes here are fairly minor IIRC (changing the order of some params in the constructor), I'll separate them in two sets of patches in the future. I just forgot about it with this patch.

> Overall I think this patch is fine, though I don't know why it is desirable.
> Some justification
> would be good; without it I tend to think that it's maybe better to keep the
> timeline subclasses
> near the code that is manipulating them.
> 

Having all this marker code all over the platform feels harder to maintain to me, and keeping them together gives us a better idea of what markers are actually currently available. Considering my suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1196294#c3 to enforce subclassing and removing the very fragile "name" string as a pseudo-type identifier, I think keeping everything in one place is helpful in the long run.

> I have no idea whether or not you need reviews from peers for all the
> affected modules.

I highly doubt that.

> 
> ::: docshell/base/nsDocShell.h
> @@ +38,2 @@
> >  #include "timeline/TimelineMarker.h"
> > +#include "timeline/JavascriptTimelineMarker.h"
> 
> Is this include really needed in the .h?  It seems like the only reference
> to the class is in the .cpp, so the include could go there instead.
> 

You're right.

> ::: docshell/base/timeline/LayerTimelineMarker.h
> @@ +22,5 @@
> > +  ~LayerTimelineMarker()
> > +  {}
> > +
> > +  virtual void AddLayerRectangles(dom::Sequence<dom::ProfileTimelineLayerRect>& aRectangles)
> > +  {
> 
> I don't think this has to be virtual any more; though see the other comment.
> 

Right again.

> Also, I'm not sure if Mozilla tries to follow "include what you use", but if
> so, then this header needs a bunch more includes.  Perhaps that's not super
> relevant in the unified build world.
> 
> ::: docshell/base/timeline/ObservedDocShell.cpp
> @@ +80,5 @@
> >          bool endIsLayerType = strcmp(endPayload->GetName(), "Layer") == 0;
> >  
> >          // Look for "Layer" markers to stream out "Paint" markers.
> >          if (startIsPaintType && endIsLayerType) {
> > +          LayerTimelineMarker* layerPayload = static_cast<LayerTimelineMarker*>(endPayload.get());
> 
> I think this is less safe than the previous approach.  Now, if someone makes
> a coding mistake somewhere, some object will be cast to the wrong type, with
> incorrect but also indeterminate results.  With the previous approach, you'd
> be guaranteed a crash.

Yup. See my previous comment about removing the fragile "name" checks and opting for a more statically sane approach. Let me know what your thoughts are about that.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

FWIW, jimb and I took the opposite approach with ubi::Node: concrete ubi::Node specializations for various types live with the types because (a) it makes includes a little simpler to have things include UbiNode.h rather than have UbiNode.h include a ton of different stuff, and more importantly (b) the specializations are more likely to stay up to date if they are by the things they need to stay in sync with because then readers are forced to realize when things get out of sync.

As far as keeping track of all the subclasses goes, dxr has pretty good search for "classes deriving from this base class" functionality.

Whatever works for you, though!

Comment 5

[Victor Porof [:vporof][:vp]]

For a concrete example, I had to use LayerTimelineMarker in ObservedDocShell.cpp, however the marker subclass was defined in FrameLayerBuilder.cpp. Thus there was no elegant way to include it. Moving it to FrameLayerBuilder.h and including *all of that* seemed even worse.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #5)

(and to clarify, a forward class declaration would not have sufficed since I had to do a cast)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4828a814212f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4828a814212f