Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

candidate fix for https://github.com/Chia-Network/clvm_tools/issues/83 #65

Merged
merged 5 commits into from
Sep 26, 2022

Conversation

prozacchiwawa
Copy link

No description provided.

trepca
trepca previously approved these changes Sep 1, 2022
Copy link

@trepca trepca left a comment

Choose a reason for hiding this comment

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

resolves the issue for me

@trepca trepca requested a review from arvidn September 1, 2022 08:51
Copy link

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

it's really hard to review with all the clippy warnings. Did you add a unit test to cover this issue? There's quite a lot of new code, so I would expect we need more than one new test case to cover it.

@prozacchiwawa
Copy link
Author

Apologies, merged up the clippy branch so these shouldn't be as noisy.

@trepca trepca requested a review from arvidn September 19, 2022 16:46
Copy link

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

I really think there should be a test demonstrating the issue with the old code and demonstrating that the new code yields the correct result.

@prozacchiwawa
Copy link
Author

The problem here is that the issue is it runs very slowly.
This code must not change anything about the output (it's frozen) so i'm unsure how to demonstrate it in a test. I can put in a cache disable and measure wallclock time I suppose.

@prozacchiwawa
Copy link
Author

how would everyone feel about a queryable cache hit metric that can be checked after compilation? (as a test)

@arvidn
Copy link

arvidn commented Sep 20, 2022

The problem here is that the issue is it runs very slowly.
This code must not change anything about the output (it's frozen) so i'm unsure how to demonstrate it in a test. I can put in a cache disable and measure wallclock time I suppose.

You're saying this is fixing a performance issue, not a correctness issue?

Reading the ticket you refer to in the title ( Chia-Network/clvm_tools#83 ) it seems to be talking about mis-compilation (which I would expect it would be easy to unit test)

@arvidn
Copy link

arvidn commented Sep 20, 2022

oh, I see that's also in the clvm_tools repository. so perhaps we don't have that issue here, just the performance.

@prozacchiwawa
Copy link
Author

ahh sorry, was thinking of the other one
apologies

…issue: a fancy way of doing 'ident' on an un-destructured module argument. the expected result is the full env we give
@prozacchiwawa
Copy link
Author

This comment is a test of my filtering change to prioritize messages from this repo.

@trepca trepca added the Ready label Sep 26, 2022
@wallentx wallentx merged commit 714f35f into base Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants