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

perf(config): minimize tbl_deep_extend #442

Closed
wants to merge 1 commit into from

Conversation

Iron-E
Copy link
Collaborator

@Iron-E Iron-E commented Apr 5, 2023

This is a ~33% performance increase to config.setup.

The diff will look better after #408 merges. For now, see 2810a0a for the real diff.

@Iron-E Iron-E added the perf Related to performance label Apr 5, 2023
@Iron-E Iron-E self-assigned this Apr 5, 2023
@romgrk
Copy link
Owner

romgrk commented Apr 5, 2023

33% of how much? I like performance but I also like simplicity, not sure if I like this tradeoff. In particular because it's only called once. Render code is called often, so I'm more enclined towards performance there than here.

This is a ~33% performance increase for `config.setup`
@Iron-E Iron-E force-pushed the perf/minimize-deep-extend branch from 2810a0a to 8c78737 Compare April 5, 2023 19:12
@Iron-E
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

0.0976ms → 0.0562ms (taken from the average of two 100,000 iteration benchmarks on my machine just now). If you don't think it's worth we can close :)

Edit: source for benchmark

@romgrk
Copy link
Owner

romgrk commented Apr 5, 2023

lol no that's definitely not worth. if it was in render code maybe because it's called constantly, but here no.

@Iron-E Iron-E closed this Apr 5, 2023
@Iron-E Iron-E deleted the perf/minimize-deep-extend branch April 5, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf Related to performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants