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

Lowering the profiler minimum sampling interval #68

Open
magenish opened this issue Jan 25, 2022 · 5 comments
Open

Lowering the profiler minimum sampling interval #68

magenish opened this issue Jan 25, 2022 · 5 comments

Comments

@magenish
Copy link

The usage of the js-self-profiler is to enable app developers to ship better interactive apps.
The definition for good interactivity UX in the industry is to response to core user interaction faster than 100ms.

As the current minimal sampling interval is 16ms/10ms windows/mac it really difficult to pinpoint the bottlenecks.
Lowering the interval to 1ms would make the api the ideal tool to drive improvement and ship first class interactivity web apps.

From security POV there shouldn't be an issue as the current high resolution time is in microsecond resolution , so 1ms interval should be fine.
@shhnjk for approving the security aspect.

@camillobruni
Copy link

There are two sides of this, I think:

  1. For high-detailed profiling I would probably recommend devtools/perfetto and local profiling (we still have the sampling interval limits in place there I think).
  2. Eventually you should get good results from profiling in the while as you could combine more samples

I agree, technically we are able to get higher sampling resolutions out of the system (especially the windows limit is quite high, and I'm pretty sure based mostly on older systems). We do have to busy-wait on windows though for high-resolution sampling as Window's SLEEP equivalent has terrible guarantees.

IMO except for security guarantees there is nothing speaking against allowing higher frequency sampling at the cost of potentially slowing down the website.

@npm1
Copy link

npm1 commented Jan 27, 2022

I don't understand why the current threshold is insufficient. Is it because you're profiling a very small percentage of users and hence not getting enough data samples? You can solve this by profiling for a longer period of time or just a slightly higher percentage of users. I would recommend looking into the performance impact of a lower threshold before changing the spec and implementation to allow it. I believe @acomminos did a lot of investigation to prove that the current threshold is acceptable, and he may have pointers on what to try.

@magenish
Copy link
Author

magenish commented Jan 27, 2022

Thanks for your reply.

Nope, we profile our entire set of users, so we have enough data.
The issue is that due to the fact the minimal sampling frequency is 16ms, that means the API would mislead the developer to think the time is being spent on the wrong functions.

Consider the following case:
image

As we are dealing with sampling profiler, it might only sample and catch function A(the red lines in my awful drawing), which will mislead the developer to investigate the wrong function.

16ms deviation is pretty big one with respect for what the industry aim to achieve for good interactive UX.

I agree lowering the minimal frequency to 1ms would cause a perf overhead(which can be said on every threshold we will choose:)), but nothing that cant be introduced by many existing APIs.
Therefore, and as the sampling frequency is already configurable by the API consumers, i dont see any reason we should constrain the API consumer to 16s specifically.

@camillobruni
Copy link

The total number of samples is the most relevant metric here.

As long as you aggregate all samples from different users you will get good resolution. Sampling will never happen at the exact same spot (it's triggered from a separate thread). So eventually you should get the complete picture.

I do agree though that we could support a higher frequency (and there are many other apis that are slow or proper footguns when used)

@shhnjk
Copy link

shhnjk commented Feb 11, 2022

Here is my investigation on whether lowering the minimal sampling interval of JS Self-Profiling API to 1 millisecond is safe:
https://github.com/shhnjk/shhnjk.github.io/blob/main/investigations/js-self-profiling/lowering_interval.md

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

No branches or pull requests

4 participants