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

[Enhancement] Timing for day construction #39

Closed
tslater2006 opened this issue Dec 7, 2020 · 5 comments · Fixed by #44
Closed

[Enhancement] Timing for day construction #39

tslater2006 opened this issue Dec 7, 2020 · 5 comments · Fixed by #44
Assignees
Labels
enhancement Enhancement of an existing feature

Comments

@tslater2006
Copy link

I think it might be useful to (optionally?) allow to capture the timings of the day constructors. Day 7 of 2020, had a lot of up front processing that both parts leveraged. instead of doing it twice, it makes sense to put in the constructor. However doing so, the Part 1 and Part 2 become a lot slimmer just leveraging the data structures built by the constructor, and I think this results in artificially low runtimes, since the bulk of the processing wasn't accounted for.

@eduherminio eduherminio added the enhancement Enhancement of an existing feature label Dec 7, 2020
@eduherminio
Copy link
Owner

That kind of makes sense @tslater2006.
Do you think we should get the time taken by the constructor added to both parts' time, or just having a separate, third measure common for both parts?
I have mixed feelings about it.

@tslater2006
Copy link
Author

Yea, i've gone back and forth on it which is why I didn't suggest a particular way :) The way I see it there are 3 choices

  1. Separate line item for construction time
  2. Gets added to both parts (this is unfair and would artificially increase your "total runtime" for SolveAll)
  3. Get added to part 1 only, so part 1's runtime is Construction + Part1, and then Part 2 is on its own.

I think my preference is 1, 3, 2. I think 1 is easier in terms of making it optional? if people don't want that value to count then they can turn it off and just see what you see today. 3 is the only other "fair" way in terms of total runtime that I can think of.

@eduherminio
Copy link
Owner

Yea, i've gone back and forth on it which is why I didn't suggest a particular way :) The way I see it there are 3 choices

😆 fair enough.

I'll give it a thought, create one/some PoCs and get back to you in the following days.
It'll remain optional and probably also disabled by default, but we'll find a way to provide those stats.

@eduherminio eduherminio self-assigned this Dec 7, 2020
@tslater2006
Copy link
Author

Sounds good, thanks for being so open to feedback/enhancements :)

@eduherminio
Copy link
Owner

The feature was relatively easy to implement, but now I'm struggling to make spectre.console cope with so many re-rendering with my own 2020's repo (🥳 that means that it's kinda performant haha).
I'll have to either explore a new way of re-rendering the results table using that library or to move (back) to some less fancy results visualization, so it may take a some time.

Meanwhile, you can test the functionality itself (and your constructors!) with this package and:
Solver.SolveAll(new SolverConfiguration { ShowConstructorElapsedTime = true, ShowTotalElapsedTimePerDay = true });

eduherminio added a commit that referenced this issue Dec 16, 2020
* Add `SolverConfiguration.ShowConstructorElapsedTime` to allow measuring constructor time (closes #39).
* Add `SolverConfiguration.ShowTotalElapsedTimePerDay` to aggregate constructor, part1 & part2 times (as it's done to calculate the mean time per day in the overall results panel).
#43 is a known bug when using both options, but isn't caused by them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants