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: Optimize isRecursive check in DynamicModelCompiler #1458

Merged
merged 2 commits into from
Mar 27, 2024

Conversation

ghostbuster91
Copy link
Contributor

@ghostbuster91 ghostbuster91 commented Mar 25, 2024

This fixes #1258

DynamicSchemaIndex.load times before and after this change for some models tested on Intel® Core™ i7-10875H × 16 64GB RAM. Testing script was mostly based on the one from the issue.

A model that consists of "aws-quicksight-spec" and "aws-sagemaker-spec".

  • before: 17 675ms
  • after: 2 659ms
  • flamegraph after:flamegraph3

A confidential model:

  • before: 12 821ms
  • after: 2 472ms
  • flamegraph after: flamegraph2

The new implementation has been tested manually with mentioned models against the old one and they both return the same results; however, it should be noted that the first model contains only a single recursive shape, while the confidential model contains only a few more.

The implementation is fully based on @Baccata's idea. My plan was to check that approach and then move to implementing mentioned Tarjan algorithm, but this implementation turned out to be quite fast. I don't mind implementing Tarjan but I propose to stop here, as at the moment the current solution is fast enough. Implementing Tarjan would probably yield better results but I think that the increase in complexity outweighs potential gains in speed. This decision can always be revisited in the future if needed.

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@ghostbuster91 ghostbuster91 marked this pull request as ready for review March 26, 2024 11:53
@Baccata Baccata merged commit 035400a into disneystreaming:series/0.18 Mar 27, 2024
11 checks passed
@ghostbuster91 ghostbuster91 deleted the perf/is-recursive branch March 27, 2024 17:59
@Baccata
Copy link
Contributor

Baccata commented Apr 4, 2024

@ghostbuster91, if it's not too much to ask, would you mind applying the same logic to the codegen side of things when you have a little time ? It may help the build-time when generating code from big models

@kubukoz
Copy link
Member

kubukoz commented Apr 4, 2024

Probably a good idea to run some profiling first as well, it's possible there are other bottlenecks in there

@kubukoz
Copy link
Member

kubukoz commented Apr 11, 2024

I made a separate ticket for the codegen part so that it's easier to track and discuss on its own: #1483

@ghostbuster91
Copy link
Contributor Author

Thanks @kubukoz
@Baccata sorry for not responding earlier - it's been a tough week. Sure, I can take a look at it, probably next week.

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

Successfully merging this pull request may close these issues.

DynamicSchemaIndex.loadModel is slow
3 participants