w3c / resource-timing

Resource Timing
https://w3c.github.io/resource-timing/
Other
121 stars 35 forks source link

Consider to limit the max size of resource timing buffer #304

Closed smaug---- closed 2 years ago

smaug---- commented 2 years ago

Somewhat recently a major social media web site used size 100000 for the buffer. That caused significant jank in the browsers. Implementations are a bit different, so methods to query the data have different performance characteristics. I had some old tests for this https://bug1688312.bmoattachments.org/attachment.cgi?id=9198744 is for getEntriesByName https://bug1688312.bmoattachments.org/attachment.cgi?id=9198778 is for getEntriesByType

getEntriesByName is very fast in Blink, getEntriesByType is very slow (which is why the getEntriesByType test does only 100 iterations). Both methods are ok-ish in Gecko. (I didn't have Webkit to test this).

Not only does the effectively endless buffer cause performance issues, but it does also lead to higher memory usage. And seemingly setResourceTimingBufferSize is rather error prone API when even the largest websites have issues caused by it.

yoavweiss commented 2 years ago

Limiting the buffer size to some reasonable value makes sense to me.

getEntriesByName is very fast in Blink, getEntriesByType is very slow

Sigh, yeah. Seems like we can and should improve it regardless.

npm1 commented 2 years ago

The demos are about user timing. Is this a bug about user timing or resource timing?

npm1 commented 2 years ago

Ping - should we move this issue to User Timing?

smaug---- commented 2 years ago

I'm not sure in which spec this should be handled. setResourceTimingBufferSize is defined in resource timing spec, which is why I filed this here.

npm1 commented 2 years ago

The title implies this is about resource timing indeed, but the demos are about user timing, so the request is a bit unclear.

npm1 commented 2 years ago

Reading this again, perhaps the request is to have a max threshold so that setResourceTimingBufferSize cannot have a crazy large size? I would argue that this is not necessary: if the developer sets this size, then they should understand that it comes at a price. Also, an entry is only created for resource fetches, so it should be very uncommon for such extremely large numbers to be reached in the first place. The issue of the performance of the methods is perhaps a separate issue which should be addressed by each user agent separately. OK to close this?

smaug---- commented 2 years ago

The APIs are very error prone. When sites like Facebook make these mistakes (and they even have a performance team, AFAIK), it hints that something should be done to the APIs.

npm1 commented 2 years ago

What maximum size would you suggest then?

nicjansma commented 2 years ago

I believe this was the original bug, which is for a site that had a very large ResourceTiming buffer size that ended up filling up and causing performance issues as they iterated through entries: https://bugzilla.mozilla.org/show_bug.cgi?id=1686930

yoavweiss commented 2 years ago

We discussed this yesterday at the WebPerfWG call, and came to a conclusion that this is maybe an implementation issue when too many entries are present. As such, it doesn't seem that limiting Resource Timing on its own would solve the implementation issue. At least IMO (with my implementer hat on), we should try to make our implementations more efficient first, before imposing restrictions on developers.

As discussed, I'm running a local experiment trying out what are the times to run getEntries() with a large number of entries from different sources. Chromium's getEntries() seems to be running at 100ms for 201K entries and at 1600ms for 2001K entries. (which implies at an O(nlgn) underlying complexity) Firefox is much faster at 20ms for 201K entries and 339ms for 2001K. I'll file Chromium bugs to improve our implementation.

@sefeng211 and @smaug---- : Since accumulating 100K RT entries is not something sites typically do (and if they would, the getEntries() call is the least of our concerns), are you OK with closing this? Seems like Firefox is already pretty fast for practical values and Chromium needs to catch up :)

bdekoz commented 2 years ago

Just to check in from the Mozilla side, it is ok to close this issue. We're still concerned about inadvertent excessive use in this API, but aren't going to die on this hill.

yoavweiss commented 2 years ago

If you're interested, I'm more than happy to introduce spec flexibility to impose an implementation defined limit. Let me know.

noamr commented 2 years ago

Closing based on previous comments. Feel free to reopen if want to pursue this.