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

Running brick in broadcast mode takes 4+ seconds #2191

Closed
twschiller opened this issue Dec 19, 2021 · 6 comments · Fixed by #2215
Closed

Running brick in broadcast mode takes 4+ seconds #2191

twschiller opened this issue Dec 19, 2021 · 6 comments · Fixed by #2215
Assignees
Labels
bug Something isn't working performance runtime
Milestone

Comments

@twschiller
Copy link
Contributor

twschiller commented Dec 19, 2021

Steps to Reproduce

  • Have 6 tabs open, 3 of which have PixieBrix active
  • Create a flow like the following
  • Click the button
  • The confetti takes 4+ seconds to appear

image

The RUN_BRICK subcall for each tab is attempting 11 times:
image

Investigation

@twschiller twschiller added bug Something isn't working runtime labels Dec 19, 2021
@twschiller twschiller added this to the 1.4.11 milestone Dec 19, 2021
@fregante
Copy link
Contributor

fregante commented Dec 19, 2021

The RUN_BRICK subcall for each tab is attempting 11 times:

That shouldn't affect the time it takes because those targets just don't exist. I'm not sure how it worked before, but probably a similar behavior existed but was silent.

Retrying is done:

  • to wait until the content script is ready
  • to wait until a named target is created and becomes ready

I think currently this retry time is around 4-5s. We can reduce the noise by limiting the broadcast to tabs we actually have access to rather than "all tabs".

@fregante
Copy link
Contributor

  • Should only be getting called on tabs PixieBrix thinks is ready:

That may be already happening, but I think that array isn't updated? So visiting a permitted host and then navigating away, still leaves the "tab ready" mark in that array if I remember correctly.

@twschiller
Copy link
Contributor Author

twschiller commented Dec 19, 2021

That may be already happening, but I think that array isn't updated? So visiting a permitted host and then navigating away, still leaves the "tab ready" mark in that array if I remember correctly.

Correct, we aren't removing tabs from tabReady on 1) full navigation, 2) closing the tab. So the tabReady just accumulates a bunch of stale information we then try to message

The good news is that the retry time is constant (or relatively constant) w.r.t. number of outdated tabs

I wonder it it’s better to try to maintain tabReady or just ping the tabs once prior to broadcast? Or somehow turn off retry in the messaging library?

@twschiller twschiller modified the milestones: 1.4.11, 1.4.12 Dec 19, 2021
@twschiller
Copy link
Contributor Author

twschiller commented Dec 19, 2021

I confirmed that the delay is also present in 1.4.10, so I won't hold up the 1.4.11 release for this

@fregante
Copy link
Contributor

Interesting! So that excludes #2090

I opened a few relates tickets, but I don’t think they're directly related to this problem. #2193 would reduce noise and potentially allow me to drop a lot of "tab ready" code, so it's worth doing soon anyway.

@fregante
Copy link
Contributor

fregante commented Dec 20, 2021

I'm not seeing a huge delay in execution, maybe less than half a second. The only issue is that it does take 4.3 seconds for the action to complete since the background handler still has to wait for the retries to end before responding. This is the log in the origin tab:

Screen Shot 5

broadcast.mov

I'll fix:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants