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

Make Journal store delayed strings, so they are computed on-demand #147

Closed
wants to merge 3 commits into from

Conversation

Porges
Copy link
Contributor

@Porges Porges commented Nov 5, 2017

This fixes the performance issues in the "greedy traversal" test (#146).

I'm not entirely sure about the change to PropertyBuilder.CounterExample, but it's not really user-visible (they would be using the counterexample 'action' instead of calling this directly).

There's another string call which is not delayed (line 275) as it didn't seem to be a performance issue.

…when demanded

This fixes the performance issues in the "greedy traversal" test.
@Porges
Copy link
Contributor Author

Porges commented Nov 5, 2017

Before:

=== TEST EXECUTION SUMMARY ===
   Hedgehog.Tests  Total: 159, Errors: 0, Failed: 0, Skipped: 0, Time: 48.931s

After:

=== TEST EXECUTION SUMMARY ===
   Hedgehog.Tests  Total: 159, Errors: 0, Failed: 0, Skipped: 0, Time: 2.287s

Copy link
Member

@moodmosaic moodmosaic left a comment

Choose a reason for hiding this comment

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

👍 /cc @jystic

@Porges
Copy link
Contributor Author

Porges commented Nov 6, 2017

Just FYI: sprintf "%A" was spending a lot of its time in reflection(!), it's not a fast call. It must have to check the object type for the various F#-specific attributes that control formatting. It would be nice if F# would provide a ToString that matched the behavior of %A for user-defined types!

@Porges
Copy link
Contributor Author

Porges commented Nov 6, 2017

... actually, that's already meant to exist: fsharp/fslang-suggestions#429

@jacobstanley
Copy link
Member

👍 This is great

@Porges
Copy link
Contributor Author

Porges commented Nov 7, 2017

Not sure why that happened twice...

moodmosaic added a commit that referenced this pull request Nov 7, 2017
Make Journal store delayed strings, so they are computed on-demand
@moodmosaic
Copy link
Member

Not sure why that happened twice...

No worries, it's now manually merged in 813e1f9 via git merge --no-ff 🎉

image

@moodmosaic moodmosaic closed this Nov 7, 2017
This was referenced Nov 7, 2017
@ghost ghost modified the milestones: 0.5.0, 0.6.0 Sep 22, 2021
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.

3 participants