Fix nested subresource load tracepoints
Created attachment 434111 [details] Patch
<rdar://problem/81031829>
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.
(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).
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.
Created attachment 434885 [details] Patch
Created attachment 435098 [details] Patch
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].