Skip to content
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.

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

Contextual and GC friendly user timing #86

Open
legendecas opened this issue Dec 13, 2021 · 12 comments
Open

Contextual and GC friendly user timing #86

legendecas opened this issue Dec 13, 2021 · 12 comments

Comments

@legendecas
Copy link
Member

legendecas commented Dec 13, 2021

As the status quo, the user marks and measures are globally buffered and need to be cleared manually with performance.clearMarks and performance.clearMeasures. In wild practice, this can be very awkward for applications or libraries that record repetitive series of operations: their marks and measures have to be prefixed to prevent from colliding from maybe overlapping async operations, and they must clear the marks and measures with prefixed names with performance.clearMarks and performance.clearMeasures after the operations complete.

If the marks and measures are not cleared correctly, they are likely to get memory leakages. This is not a very idiom JavaScript resource management pattern. It can be better if those buffered entries can be garbage collected once their contextual async operation is not referenced anymore.

This problem stands out on Node.js significantly. Node.js provides standard-compliant user timing API to allow applications or libraries to work with Web platforms seamlessly, whilst those applications or libraries are supposed to run code repetitively on the server-side.

If the user timing can be created under a contextual performance timeline, then this specific performance timeline can be referenced from the context of these async operations, like events, requests, etc. Once the context of these operations was not used anymore, all of them can be garbage collected cleanly. And there won't need the name prefixes for entries since they are not put into the global performance timeline and won't get overlapping with each other.

Conceptual usage:

addEventListener('an_event_name', event => {
  const eventPerformance = new Performance('click event');
  eventPerformance.mark('mark a');
  asyncOperation()
    .then(() => {
      eventPerformance.measure('measure a', 'mark a');
      logEntries(eventPerformance.getEntries());
      // eventPerformance and their user timing can be GC-ed without manual clearance.
    });
});
@yoavweiss
Copy link
Contributor

So, you're suggesting to enable creating temporary user-held Performance objects, and once they are GCed, all their entries can similarly be GCed? That's interesting, but at the same time, quite a change from the way the performance timeline/observers currently work.

Can't you get the same effect by registering PerformanceObservers for the user timings you care about, and clear those user timing entries once you logged them?

@legendecas
Copy link
Member Author

legendecas commented Dec 29, 2021

@yoavweis That's a good point. However, using PerformanceObservers to clear those user timing entries doesn't resolve the entry names overlapping problem on instrumenting async operations mentioned in the OP. Those async operations may overlap, like:

1. async op start (series 1)
2. mark start (entry 1)
...
3. async op start (series 2)
4. mark start (entry 2)
...
5. async op end (series 1)
6. mark end (entry 3)
7. measure start to end (this will be resolved as entry 2 to entry 3, instead of entry 1 to entry 3)
...
7. async op end (series 2)
8. mark end (entry 4)

That's why I mentioned in the OP that user timing has to prefix entry names to compute correct measures in async operations. However, the current performance API to get the entry list doesn't work well with this pattern. We either have to get the list by PerformanceObserverEntryList.getEntriesByName with full name or
PerformanceObserverEntryList.getEntries() to get full list. The same problem also applies on performance.clearMarks().

So I'm suggesting creating a stand-alone user timing performance timeline for each async operation (performance.measure can resolve in a separate timeline/namespace), and the timeline can be garbage collected with a common JavaScript resource management pattern.

@yoavweiss
Copy link
Contributor

I wonder if adding an option that enables you to clear marks and measures by passing an array of PerformanceMark/PerformanceMeasure (rather than clearing by name) would enable you to decouple the entries' GC from their names, and would avoid the need for book-keeping or name prefixing.

That way, you'd be able to have a PerfObserver for them, write down the results for the entries you get, and then clear them by adding them to an array and passing that array to a clearing method. WDYT?

@legendecas
Copy link
Member Author

... and would avoid the need for book-keeping or name prefixing.

I'm a bit confused about this statement, would you elaborate on how the approach would avoid name prefixing for performance.measure(start, end) in the example above? It seems to me it still needs to keep a unique name for each 'start' entry.

@yoavweiss
Copy link
Contributor

We may also need to add a Performance.measure() version that takes in two PerformanceMark entries as input, rather than names, to enable that.

@legendecas
Copy link
Member Author

@yoavweiss Thanks for point that out! I'd agree this approach is more easier to be adopted and fit in the current shape of design.

Before we make changes on platforms like Node.js, is there anything/process should be done on the spec? I'd be happy to take it and push it forward.

@yoavweiss
Copy link
Contributor

It would be good to discuss this first at a WebPerfWG meeting, see that there are no objections from current implementers or users, and that there's appetite to make this change. The next step after that is to define those new API methods and their processing model.

@yoavweiss
Copy link
Contributor

This was discussed at a WG meeting a couple of months back.

Summary:

  • Currently it’s hard to manage GC lifetimes of user-timing entries without clearing all of them. It’s also impossible for tools to highlight some user timings but not others.
  • The issue mostly talked about the former, but there was strong consensus on the call that adding namespaces to User Timing would solve both issues.

@legendecas
Copy link
Member Author

Considering how the PerformanceObserver and Performanceinterfaces should adopt the namespace things, I think adding a new field namespace (or namely tag) to the PerformanceEntry would be a most feasible solution.

Creating new Performance instances may confuses the scopes of globally registered PerformanceObserver -- should it observe entries created in these user created Performance instances, how should it declare its interests on these Performance, etc.

Rather, adding a new field tag/namespace to the PerformanceEntry can be more intuitive to the current shape of API:

addEventListener('an_event_name', event => {
  const tag = getPerformanceTag(event); // <= any string
  performance.mark('mark a', { tag });
  asyncOperation()
    .then(() => {
      performance.measure('measure a', 'mark a', { tag });
      logEntries(performance.getEntriesByTag(tag));
      performance.clearMarksByTag(tag);
      performance.clearMeasuresByTag(tag);
    });
});

const observer = new PerformanceObserver(list => {
  const entries = list.getEntries();
  for (let idx = 0; idx < entries.length; idx++) {
    // process entries
  }
});
observer.observe({ entryTags: [ "my-tag" ] });

WDYT?

@yoavweiss
Copy link
Contributor

It's unclear to me what tags/namespaces would mean outside the context of user timing. (and what getPerformanceTag is supposed to do in your example)

@mmocny
Copy link

mmocny commented Apr 28, 2022

I think Tags/namespaces are uniquely important and so no concerns about this proposal to add explicit support for tagging. I especially like that this can be a shared convention in tooling, and that the extra filter in .observe() call can be useful.

However, I wonder if the byTag() helpers are really necessary compared to just using normal .filter(). I think we try to move away from performance.getEntries* APIs towards PerformanceObserver if possible, anyway. It may also be hard to support all possible filter criteria beyond tags.

Perhaps there is a significant perf win to doing it with a explicit API and tagging would seem like a top candidate... but maybe we can do a quick perf test? If you had 1000 marks, how long does it take to get them all and filter? What is a reasonable upper limit for user timing sizes?

Specifically it seems that .clearMarks/measures API right now is fairly limited, but perhaps we can add support for passing a reference to PerformanceEntry? Then you can performance.clearMarks( performance.getEntriesByType("mark").filter(entry => ...) )

Finally, how commonly is clearing marks and measures actually done? I know one of the use cases is to create custom timings in Developer tooling, but without reporting to performance timeline (#68)... but I think that issues needs a holistic solution. With user timing L3, you really don't need to create a bunch of marks for custom measure times... the example above could have been:

addEventListener('an_event_name', event => {
  const tag = getPerformanceTag(event); // <= any string
  const start = performance.now();
  asyncOperation()
    .then(() => {
      performance.measure('measure a', { start, tag ); // end is optional
    });
});

const observer = new PerformanceObserver(list => {
  logEntries(list.getEntries()); // You may want to filter further, or report some marks as well
  // Optionally, clear these
});
observer.observe({ type: "measure", entryTag: "tag" });

@nicjansma
Copy link

nicjansma commented May 11, 2022

Discussed on Apr 28 WebPerf WG call: https://w3c.github.io/web-performance/meetings/2022/2022-04-28/index.html

Summary:

  • String and symbol can both fit in the tag types.
  • Generic filtering/clearing methods can be more extensible.
  • Tags can be beneficial to timings beyond user timing, e.g. EventTiming

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants