Scheduling Profiler: Improve import performance by dropping IE support #19872
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5fc7cac:
|
taneliang
changed the title
bvaughn
suggested changes
Sep 21, 2020
| } else { | ||
| targets.chrome = minChromeVersion.toString(); | ||
| targets.firefox = minFirefoxVersion.toString(); | ||
| // We won't support IE because that'll double profile import times. |
There was a problem hiding this comment.
It's unfortunate that we have to "support" IE for the case of RN, particularly if this results in less optimal compiled code. Maybe we can address this in a more wholistic way.
- I'll follow up with the Hermes team to see if this is still the best way to target Hermes for the standalone DevTools' embedded backend.
- I'll post a PR (shortly) that kills this target for everything except that standalone backend. (That seems better than leaving it on for everything except the scheduling profiler.)
There was a problem hiding this comment.
Makes sense, thanks!
|
Thanks for digging in and identifying the cause of this perf problem, @taneliang! 🙇 |
|
Closing in favor of #19875 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #19759 (comment) by forking the shared DevTools Babel config and disabling IE support. I think this should be okay for our standalone app, but we'll likely hit this perf issue again when this is merged into the main DevTools.
cc @bvaughn
Test Plan
Manually tested with our 57MB facebook.com profile.
Current https://react-scheduling-profiler.vercel.app/ (deployed from https://github.com/MLH-Fellowship/scheduling-profiler-prototype):
master:This branch. It's about 5%-15% slower than https://react-scheduling-profiler.vercel.app/:
Alternate approach (b48ab94): reducing supported browsers to just
last 2 Chrome/Firefox versions. This has a runtime similar to https://react-scheduling-profiler.vercel.app/: