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

Temporary fix to speed up canAvoidInstr #1699

Merged
merged 14 commits into from
Apr 29, 2024

Conversation

DiligentPenguinn
Copy link
Contributor

Following the fact that the check canAvoidInstr implemented in previous PR #1687 is too slow, this PR implements a temporary fix by caching the result of isEnvDependent as a field for each ControlItem

@DiligentPenguinn DiligentPenguinn added the critical Fixing this is mission-critical label Apr 27, 2024
@coveralls
Copy link

coveralls commented Apr 27, 2024

Pull Request Test Coverage Report for Build 8881462669

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 29 of 30 (96.67%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 82.112%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/cse-machine/utils.ts 18 19 94.74%
Totals Coverage Status
Change from base Build 8871900610: 0.03%
Covered Lines: 10930
Relevant Lines: 12966

💛 - Coveralls

@DiligentPenguinn DiligentPenguinn marked this pull request as draft April 27, 2024 15:41
Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for a hotfix, thanks!

Though I'd remove isEnvDependent entirely and move the logic to the node creator function instead. What do you think? Do you foresee any more changes to this logic? You mentioned this is just a basic check (for the demo the other day) right? What future more "advanced" checks might there be?

Also, can I check what's left of this PR before it can be marked as ready?

Thanks!

@DiligentPenguinn
Copy link
Contributor Author

DiligentPenguinn commented Apr 27, 2024

@RichDom2185 Yes, I think that moving the logic to the node creator function is what should ultimately be done (for now this is just a basic, non-exhaustive check). but I won't be able to work on it very soon (due to exams). Until then I think it is still considerably slow with this fix though so I'm trying other ways to speed it up a bit more first. If you think it's fine for now then I'll mark it as ready

@DiligentPenguinn DiligentPenguinn marked this pull request as ready for review April 27, 2024 17:05
@DiligentPenguinn DiligentPenguinn marked this pull request as draft April 27, 2024 17:40
@DiligentPenguinn DiligentPenguinn marked this pull request as ready for review April 27, 2024 17:48
@DiligentPenguinn
Copy link
Contributor Author

DiligentPenguinn commented Apr 27, 2024

@RichDom2185 I just pushed a second approach, this seems to fix the efficiency issue, and I think it's closer to what would be done when we move the logic to the node creator function eventually.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor nits, otherwise all good, thanks a lot!

src/cse-machine/interpreter.ts Outdated Show resolved Hide resolved
src/cse-machine/interpreter.ts Outdated Show resolved Hide resolved
src/cse-machine/interpreter.ts Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this as long as it is a temporary hotfix. Please create an issue to migrate this to the node creator in the future.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1702.

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge this first so that the other CI workflows can complete quickly…

@RichDom2185 RichDom2185 enabled auto-merge (squash) April 29, 2024 15:47
@RichDom2185
Copy link
Member

Part of #1698 (does not close it fully due to the comment above).

@RichDom2185 RichDom2185 enabled auto-merge (squash) April 29, 2024 15:48
@RichDom2185 RichDom2185 merged commit f74c38f into master Apr 29, 2024
4 checks passed
@RichDom2185 RichDom2185 deleted the cse-machine-simplify-env-instr branch April 29, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Fixing this is mission-critical
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants