Skip to content

Use a hashset - #2853

Merged
forki merged 1 commit into
masterfrom
whyhashset
Oct 18, 2017
Merged

forki merged 1 commit into
masterfrom
whyhashset

Conversation

@forki

@forki forki commented Oct 18, 2017 •

Copy link
Copy Markdown
Member

references #2844

brings original why sample down to 1s

@forki

forki commented Oct 18, 2017 •

Copy link
Copy Markdown
Member Author

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

@theimowski

Copy link
Copy Markdown
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?

@forki

forki commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

@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
@theimowski

theimowski commented Oct 18, 2017 •

Copy link
Copy Markdown
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?

@forki

forki commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

so revert?

@forki
forki deleted the whyhashset branch October 18, 2017 12:50
@theimowski

Copy link
Copy Markdown
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

@forki

forki commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

@matthid ?

@theimowski

Copy link
Copy Markdown
Member

Do we know how does yarn handle such cases?

@forki

forki commented Oct 18, 2017

Copy link
Copy Markdown
Member Author

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

@theimowski

Copy link
Copy Markdown
Member

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants