Skip to content

Remove PerformanceEntry.id and rephrase id generation. #219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: gh-pages
Choose a base branch
from
Open

Remove PerformanceEntry.id and rephrase id generation. #219

wants to merge 10 commits into from

Conversation

@powdercloud
Copy link

@powdercloud powdercloud commented Mar 18, 2025 •
edited by pr-preview bot
Loading

  • Removing PerformanceEntry.id, since it's shadowed by PerformanceElementTiming.id, see PerformanceEntry.id is shadowed by PerformanceElementTiming.id. #218
  • Rephrasing how navigation ids are generated:
    • no more mention of id, but the mechanism is the same as before, a random navigation id for each new document, which is then incremented for navigations that don't change the document
    • changes the order of the sections for queing so that it first talks about navigation performance entries, since these change the navigation ids, and only after that other entries

Preview | Diff

@powdercloud
Remove PerformanceEntry.id and rephrase id generation.
8308e97 
- Removing PerformanceEntry.id, since it's shadowed by
  PerformanceElementTiming.id, see
  w3c#218
- Rephrasing how navigation ids are generated:
  - no more mention of id, but the mechanism is the same
    as before, a random navigation id for each new document, which
    is then incremented for navigations that don't
    change the document
  - changes the order of the sections for queing so that it
    first talks about navigation performance entries,
    since these change the navigation ids, and only
    after that other entries
@powdercloud
typo
cc34a39
mmocny
mmocny reviewed Mar 19, 2025
View reviewed changes
Copy link

@mmocny mmocny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit: swapping the order of the algorithms, while cleaner, makes it slightly harder to review the changes in isolation. Would it be possible to keep the order for initial review?)

I think the change generally looks good. However, looking at the wording, I wonder if we really need a "most recent navigation" value which is set to a specific navigation entry.

This has a few downsides:

  • Requires a mostly similar redundant copy of the queue and entry algorithm
  • Requires that we increment navigationId only after we finalize the navigation entry. We might want to increment it before the navigation timing is fully known (in theory).

What if we just have a "current navigationId" which is always default initialized to a random number, always incremented by a small integer, and some other spec calls into a simpler "increment navigation id" algorithm or some such thing?

@powdercloud
Copy link
Author

powdercloud commented Mar 19, 2025

I swapped the sections back for now, so it looks that in Github's file view it's now easy to see a diff of the modifications relative to the 'production' version.
I think with the wording, you're right, it's probably just the most recent navigation id that matters here. I think this is also what I'm seeing in the Chromium change that I'm preparing actually, the entry isn't kept around, just the id.
And, there may be one more state that can occur, when there isn't a document yet - in the code I'm using 0 thus far in that case (for 'uninitialized'?), but I don't know yet whether that value ever actually surfaces via the API (so it wouldn't need to be in the spec, just an implementation wrinkle). If it's cool, I'll come back to this spec update in a little while with more knowledge (1-2 days).

@powdercloud
Copy link
Author

powdercloud commented Mar 21, 2025

I uploaded a new version, main changes are:

  • In the initial section, as suggested, specified "current navigation id" and "current document navigation id", both initialized to 0.
  • In the IDL for PerformanceEntry, revised the type of navigationId to long (meaning a signed int32). I think this is better for json and javascript APIs; the extra range of unsigned long long isn't needed and may cause clipping / loss of precision (e.g. the 54 bit limit) and/or things getting emitted as floating point numbers with a decimal dot.
  • Revised Generate a navigation id to use current navigation id and current document navigation id.
  • Merged the case of queing a navigation performance entry into Queue a PerformanceEntry.

@mmocny
Copy link

mmocny commented Jun 4, 2025

Sorry for the delay on review here. I scanned through, and I do like the change.

Before reviewing the wording, can you remind me: Did you find when/why PerformanceEntry.id was added in the first place? Specifically I'm curious if:

  • Was this accidentally added which trying to add support for navigationId, and we should clean it up, or
  • Was this a really old part of PerformanceEntry spec that has been forgotten about (and not implemented in Chromium) but perhaps has some value on some other platform?

aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 12, 2025
@powdercloud
[soft navs] Make PerformanceEntry.navigationId a number.
c701061 
https://w3c.github.io/performance-timeline/ specifies navigationId to be
a number, whereas Chromium currently implements it as a string. This
change fixes that. The "algorithm" for making a navigation id is
implemented in navigation_id_generator.cc, with commentary and unittest.
Updating the spec remains a todo after this lands.

Notes:

1) In largest_contentful_paint_calculator, there's a pre-existing
"navigationId" in for the performance trace entries. I'm leaving this
alone (slightly undoing the mixup there), but adding the performance
timeline spec related ID as performanceTimelineNavigationId. The idea is
that this will allow users of the trace to find corresponding
PerformanceEntry instances e.g. from RUM data.

2) For the IDL file, the field is unsigned long long, just like it is in
the spec. However, the C++ implementation uses uint32_t, because values
above std::limits<int32_t>::max() don't make much sense for Javascript /
JSON APIs.

3) I'll be preparing a Github PR to align the spec and this change
w3c/performance-timeline#219; the PR needs more
updating after this edit, so please consider that WIP for now.

Implementation Details:

The (simple) algorithm for randomly creating and incrementing navigation
ids is in navigation_id_generator{.h,.cc,_test.cc}.

It's a field of window_performance.{h,cc}, which is convenient in that
hard navigations that would reset the performance object will also
create a randomly initialized navigation id, whereas for soft
navigations (and bfcache restores if enabled), we'll trigger the small
increment. There's also a performance object that's not associated with
a local dom window, and this will dole out the default navigation id
value, which is 0.

One of the constructors for PerformanceEntry now delegates to the other one; and both have the navigation_id
parameter there, and then invoking that constructor by all subclasses,
passing in the navigation id. This is relatively simple and better than
getting it from the local dom window, because these things get
instantiated by a performance object / when there's a performance object
around, which now has the navigation id generator as a field.

I added a new test,
third_party/blink/web_tests/http/tests/inspector-protocol/tracing/performance-entry-navigation-id.js;
this exercises the navigation id for situations that we care about. We
actually don't care about situations with empty documents, because they
don't involve interesting performance entries.

To answer the question what happens with opening a window and then
reusing that for a document, I added two lines to
PerformanceLifetimeTest.SurviveContextSwitch in
window_performance_test.cc.

Bug: 421337393
Change-Id: Ia669a3f3eb201d491dafd49e488aa69558ecfd99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6604387
Reviewed-by: Michal Mocny <mmocny@chromium.org>
Reviewed-by: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1485889}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 12, 2025
@powdercloud @chromium-wpt-export-bot
[soft navs] Make PerformanceEntry.navigationId a number.
18b29b7 
https://w3c.github.io/performance-timeline/ specifies navigationId to be
a number, whereas Chromium currently implements it as a string. This
change fixes that. The "algorithm" for making a navigation id is
implemented in navigation_id_generator.cc, with commentary and unittest.
Updating the spec remains a todo after this lands.

Notes:

1) In largest_contentful_paint_calculator, there's a pre-existing
"navigationId" in for the performance trace entries. I'm leaving this
alone (slightly undoing the mixup there), but adding the performance
timeline spec related ID as performanceTimelineNavigationId. The idea is
that this will allow users of the trace to find corresponding
PerformanceEntry instances e.g. from RUM data.

2) For the IDL file, the field is unsigned long long, just like it is in
the spec. However, the C++ implementation uses uint32_t, because values
above std::limits<int32_t>::max() don't make much sense for Javascript /
JSON APIs.

3) I'll be preparing a Github PR to align the spec and this change
w3c/performance-timeline#219; the PR needs more
updating after this edit, so please consider that WIP for now.

Implementation Details:

The (simple) algorithm for randomly creating and incrementing navigation
ids is in navigation_id_generator{.h,.cc,_test.cc}.

It's a field of window_performance.{h,cc}, which is convenient in that
hard navigations that would reset the performance object will also
create a randomly initialized navigation id, whereas for soft
navigations (and bfcache restores if enabled), we'll trigger the small
increment. There's also a performance object that's not associated with
a local dom window, and this will dole out the default navigation id
value, which is 0.

One of the constructors for PerformanceEntry now delegates to the other one; and both have the navigation_id
parameter there, and then invoking that constructor by all subclasses,
passing in the navigation id. This is relatively simple and better than
getting it from the local dom window, because these things get
instantiated by a performance object / when there's a performance object
around, which now has the navigation id generator as a field.

I added a new test,
third_party/blink/web_tests/http/tests/inspector-protocol/tracing/performance-entry-navigation-id.js;
this exercises the navigation id for situations that we care about. We
actually don't care about situations with empty documents, because they
don't involve interesting performance entries.

To answer the question what happens with opening a window and then
reusing that for a document, I added two lines to
PerformanceLifetimeTest.SurviveContextSwitch in
window_performance_test.cc.

Bug: 421337393
Change-Id: Ia669a3f3eb201d491dafd49e488aa69558ecfd99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6604387
Reviewed-by: Michal Mocny <mmocny@chromium.org>
Reviewed-by: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1485889}
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Jul 12, 2025
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 12, 2025
@powdercloud @chromium-wpt-export-bot
[soft navs] Make PerformanceEntry.navigationId a number.
182be03 
https://w3c.github.io/performance-timeline/ specifies navigationId to be
a number, whereas Chromium currently implements it as a string. This
change fixes that. The "algorithm" for making a navigation id is
implemented in navigation_id_generator.cc, with commentary and unittest.
Updating the spec remains a todo after this lands.

Notes:

1) In largest_contentful_paint_calculator, there's a pre-existing
"navigationId" in for the performance trace entries. I'm leaving this
alone (slightly undoing the mixup there), but adding the performance
timeline spec related ID as performanceTimelineNavigationId. The idea is
that this will allow users of the trace to find corresponding
PerformanceEntry instances e.g. from RUM data.

2) For the IDL file, the field is unsigned long long, just like it is in
the spec. However, the C++ implementation uses uint32_t, because values
above std::limits<int32_t>::max() don't make much sense for Javascript /
JSON APIs.

3) I'll be preparing a Github PR to align the spec and this change
w3c/performance-timeline#219; the PR needs more
updating after this edit, so please consider that WIP for now.

Implementation Details:

The (simple) algorithm for randomly creating and incrementing navigation
ids is in navigation_id_generator{.h,.cc,_test.cc}.

It's a field of window_performance.{h,cc}, which is convenient in that
hard navigations that would reset the performance object will also
create a randomly initialized navigation id, whereas for soft
navigations (and bfcache restores if enabled), we'll trigger the small
increment. There's also a performance object that's not associated with
a local dom window, and this will dole out the default navigation id
value, which is 0.

One of the constructors for PerformanceEntry now delegates to the other one; and both have the navigation_id
parameter there, and then invoking that constructor by all subclasses,
passing in the navigation id. This is relatively simple and better than
getting it from the local dom window, because these things get
instantiated by a performance object / when there's a performance object
around, which now has the navigation id generator as a field.

I added a new test,
third_party/blink/web_tests/http/tests/inspector-protocol/tracing/performance-entry-navigation-id.js;
this exercises the navigation id for situations that we care about. We
actually don't care about situations with empty documents, because they
don't involve interesting performance entries.

To answer the question what happens with opening a window and then
reusing that for a document, I added two lines to
PerformanceLifetimeTest.SurviveContextSwitch in
window_performance_test.cc.

Bug: 421337393
Change-Id: Ia669a3f3eb201d491dafd49e488aa69558ecfd99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6604387
Reviewed-by: Michal Mocny <mmocny@chromium.org>
Reviewed-by: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1485889}
lando-prod-mozilla bot pushed a commit to mozilla-firefox/firefox that referenced this pull request Jul 14, 2025
@powdercloud @moz-wptsync-bot
Bug 1977013 [wpt PR 53736] - [soft navs] Make PerformanceEntry.naviga…
525cedc 
…tionId a number., a=testonly

Automatic update from web-platform-tests
[soft navs] Make PerformanceEntry.navigationId a number.

https://w3c.github.io/performance-timeline/ specifies navigationId to be
a number, whereas Chromium currently implements it as a string. This
change fixes that. The "algorithm" for making a navigation id is
implemented in navigation_id_generator.cc, with commentary and unittest.
Updating the spec remains a todo after this lands.

Notes:

1) In largest_contentful_paint_calculator, there's a pre-existing
"navigationId" in for the performance trace entries. I'm leaving this
alone (slightly undoing the mixup there), but adding the performance
timeline spec related ID as performanceTimelineNavigationId. The idea is
that this will allow users of the trace to find corresponding
PerformanceEntry instances e.g. from RUM data.

2) For the IDL file, the field is unsigned long long, just like it is in
the spec. However, the C++ implementation uses uint32_t, because values
above std::limits<int32_t>::max() don't make much sense for Javascript /
JSON APIs.

3) I'll be preparing a Github PR to align the spec and this change
w3c/performance-timeline#219; the PR needs more
updating after this edit, so please consider that WIP for now.

Implementation Details:

The (simple) algorithm for randomly creating and incrementing navigation
ids is in navigation_id_generator{.h,.cc,_test.cc}.

It's a field of window_performance.{h,cc}, which is convenient in that
hard navigations that would reset the performance object will also
create a randomly initialized navigation id, whereas for soft
navigations (and bfcache restores if enabled), we'll trigger the small
increment. There's also a performance object that's not associated with
a local dom window, and this will dole out the default navigation id
value, which is 0.

One of the constructors for PerformanceEntry now delegates to the other one; and both have the navigation_id
parameter there, and then invoking that constructor by all subclasses,
passing in the navigation id. This is relatively simple and better than
getting it from the local dom window, because these things get
instantiated by a performance object / when there's a performance object
around, which now has the navigation id generator as a field.

I added a new test,
third_party/blink/web_tests/http/tests/inspector-protocol/tracing/performance-entry-navigation-id.js;
this exercises the navigation id for situations that we care about. We
actually don't care about situations with empty documents, because they
don't involve interesting performance entries.

To answer the question what happens with opening a window and then
reusing that for a document, I added two lines to
PerformanceLifetimeTest.SurviveContextSwitch in
window_performance_test.cc.

Bug: 421337393
Change-Id: Ia669a3f3eb201d491dafd49e488aa69558ecfd99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6604387
Reviewed-by: Michal Mocny <mmocny@chromium.org>
Reviewed-by: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1485889}

--

wpt-commits: 182be033eb63fdcfb4f20f983d2e4213bbf47a0a
wpt-pr: 53736
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jul 15, 2025
@powdercloud @moz-wptsync-bot
Bug 1977013 [wpt PR 53736] - [soft navs] Make PerformanceEntry.naviga…
16bf2e5 
…tionId a number., a=testonly

Automatic update from web-platform-tests
[soft navs] Make PerformanceEntry.navigationId a number.

https://w3c.github.io/performance-timeline/ specifies navigationId to be
a number, whereas Chromium currently implements it as a string. This
change fixes that. The "algorithm" for making a navigation id is
implemented in navigation_id_generator.cc, with commentary and unittest.
Updating the spec remains a todo after this lands.

Notes:

1) In largest_contentful_paint_calculator, there's a pre-existing
"navigationId" in for the performance trace entries. I'm leaving this
alone (slightly undoing the mixup there), but adding the performance
timeline spec related ID as performanceTimelineNavigationId. The idea is
that this will allow users of the trace to find corresponding
PerformanceEntry instances e.g. from RUM data.

2) For the IDL file, the field is unsigned long long, just like it is in
the spec. However, the C++ implementation uses uint32_t, because values
above std::limits<int32_t>::max() don't make much sense for Javascript /
JSON APIs.

3) I'll be preparing a Github PR to align the spec and this change
w3c/performance-timeline#219; the PR needs more
updating after this edit, so please consider that WIP for now.

Implementation Details:

The (simple) algorithm for randomly creating and incrementing navigation
ids is in navigation_id_generator{.h,.cc,_test.cc}.

It's a field of window_performance.{h,cc}, which is convenient in that
hard navigations that would reset the performance object will also
create a randomly initialized navigation id, whereas for soft
navigations (and bfcache restores if enabled), we'll trigger the small
increment. There's also a performance object that's not associated with
a local dom window, and this will dole out the default navigation id
value, which is 0.

One of the constructors for PerformanceEntry now delegates to the other one; and both have the navigation_id
parameter there, and then invoking that constructor by all subclasses,
passing in the navigation id. This is relatively simple and better than
getting it from the local dom window, because these things get
instantiated by a performance object / when there's a performance object
around, which now has the navigation id generator as a field.

I added a new test,
third_party/blink/web_tests/http/tests/inspector-protocol/tracing/performance-entry-navigation-id.js;
this exercises the navigation id for situations that we care about. We
actually don't care about situations with empty documents, because they
don't involve interesting performance entries.

To answer the question what happens with opening a window and then
reusing that for a document, I added two lines to
PerformanceLifetimeTest.SurviveContextSwitch in
window_performance_test.cc.

Bug: 421337393
Change-Id: Ia669a3f3eb201d491dafd49e488aa69558ecfd99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6604387
Reviewed-by: Michal Mocny <mmocny@chromium.org>
Reviewed-by: Scott Haseley <shaseley@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1485889}

--

wpt-commits: 182be033eb63fdcfb4f20f983d2e4213bbf47a0a
wpt-pr: 53736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mmocny mmocny mmocny left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@powdercloud @mmocny