Bug 228242 – Fix nested resource load tracepoint intervals

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

Fix nested subresource load tracepoints

[Ben Nham]

Created attachment 434111 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81031829>

[Alex Christensen]

Comment on attachment 434111 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2561
> +                tracePoint(MainResourceLoadDidEnd, reinterpret_cast<uint64_t>(this));

The page ID and frame ID should be sufficient here.  Why didn't you include them at all?

> Source/WebCore/loader/SubresourceLoader.cpp:745
> +        tracePoint(SubresourceLoadDidEnd, reinterpret_cast<uint64_t>(this));

SubresourceLoader has .identifier() which is probably even more reliable than this because our allocator can reuse slots of memory.

[Ben Nham]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 434111 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434111&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:2561
> > +                tracePoint(MainResourceLoadDidEnd, reinterpret_cast<uint64_t>(this));
> 
> The page ID and frame ID should be sufficient here.  Why didn't you include
> them at all?

They're already passed to the beginning tracePoint call (MainResourceLoadDidStartProvisional). The reason why we pass the pageID and frameID there is so that the tracing UI displays those arguments in a nice fashion. There's no need to pass the pageID and frameID to the MainResourceLoadDidEnd tracePoint since they were already passed to the MainResourceLoadDidStartProvisional that marks the beginning of the interval.

The only state you need to pass to the end tracepoint is whatever data you need for disambiguating nested intervals, which is the loader address in this case.

> > Source/WebCore/loader/SubresourceLoader.cpp:745
> > +        tracePoint(SubresourceLoadDidEnd, reinterpret_cast<uint64_t>(this));
> 
> SubresourceLoader has .identifier() which is probably even more reliable
> than this because our allocator can reuse slots of memory.

It's okay if the loader address is reused as long as there aren't two subresource loads in flight at the same time in the same process that use the same loader, which as far as I can tell isn't possible. Using the subresource loader address instead of the (pageID, resourceID) tuple for disambiguating nested intervals means one less argument to pass to the SubresourceLoadDidEnd tracePoint call (not that that's a big deal one way or the other).

[Alex Christensen]

I still don't think it's great practice to cast a pointer to an integer identifier, especially when that object already has a unique integer identifier.

[Ben Nham]

Created attachment 434885 [details]
Patch

[Ben Nham]

Created attachment 435098 [details]
Patch

[EWS]

Committed r280859 (240400@main): <https://commits.webkit.org/240400@main>

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