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

Use a hashset #2853

Merged
merged 1 commit into from Oct 18, 2017

Conversation

Projects
None yet
2 participants
@forki
Member

forki commented Oct 18, 2017

references #2844

brings original why sample down to 1s

@forki forki requested a review from theimowski Oct 18, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 18, 2017

Member

so from 21s down to 1s via 3 PRs - good team work @theimowski!

Member

forki commented Oct 18, 2017

so from 21s down to 1s via 3 PRs - good team work @theimowski!

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 18, 2017

Member

LGTM, pity there are no tests to be sure there's no regression.
Can we name the nested rec function paths' to better distinguish from outer fun?

Member

theimowski commented Oct 18, 2017

LGTM, pity there are no tests to be sure there's no regression.
Can we name the nested rec function paths' to better distinguish from outer fun?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 18, 2017

Member

@theimowski my question is: are we ver going back here? In my optimization we only add to the set and never reuse old sets.

Member

forki commented Oct 18, 2017

@theimowski my question is: are we ver going back here? In my optimization we only add to the set and never reuse old sets.

@forki forki merged commit 4d8442a into master Oct 18, 2017

1 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 18, 2017

Member

Hm crap you're right. Now it might skip some chains ...
E.g. when we have a diamond:

A
  B
B
  C
  D
C
  E
D
  E
E

Then it would output just A->B->C->E and skip A->B->D->E because B was already added to set
So it changes behavior - but aside maybe such behavior is ok?

Member

theimowski commented Oct 18, 2017

Hm crap you're right. Now it might skip some chains ...
E.g. when we have a diamond:

A
  B
B
  C
  D
C
  E
D
  E
E

Then it would output just A->B->C->E and skip A->B->D->E because B was already added to set
So it changes behavior - but aside maybe such behavior is ok?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 18, 2017

Member

so revert?

Member

forki commented Oct 18, 2017

so revert?

@forki forki deleted the whyhashset branch Oct 18, 2017

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 18, 2017

Member

That's up to you - behavior changed, but I'm not sure whether it's ok or not.
By that I mean the output shown when --details is specified might get very long for bigger trees

Member

theimowski commented Oct 18, 2017

That's up to you - behavior changed, but I'm not sure whether it's ok or not.
By that I mean the output shown when --details is specified might get very long for bigger trees

@forki

This comment has been minimized.

Show comment
Hide comment
@forki
Member

forki commented Oct 18, 2017

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 18, 2017

Member

Do we know how does yarn handle such cases?

Member

theimowski commented Oct 18, 2017

Do we know how does yarn handle such cases?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 18, 2017

Member

Dunno. But I'm fine with revert. 3s is still usable.

Member

forki commented Oct 18, 2017

Dunno. But I'm fine with revert. 3s is still usable.

@theimowski

This comment has been minimized.

Show comment
Hide comment
@theimowski

theimowski Oct 18, 2017

Member

Ok then how about revert and create issue to cover the code with some tests and optionally then improve perf

Member

theimowski commented Oct 18, 2017

Ok then how about revert and create issue to cover the code with some tests and optionally then improve perf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment