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

Refactor the DG #25

Merged
merged 10 commits into from
Jul 26, 2015
Merged

Refactor the DG #25

merged 10 commits into from
Jul 26, 2015

Conversation

segiddins
Copy link
Member

Does this look any faster?

@alloy
Copy link
Member

alloy commented Jul 23, 2015

Have you done some real-world benchmarks?

@segiddins
Copy link
Member Author

It seems to keep most things the same, but makes rubygems/bundler#3803 over an order of magnitude faster

end
attr_accessor :incoming_edges
# graph.edges.select { |e| e.destination.equal?(self) }
# end

Choose a reason for hiding this comment

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

Delete the commented out code?

@indirect
Copy link

In general, I can believe this is faster on huuuuuuge graphs because it looks like this creates less sets and has less object churn? But my random guesses seem less important than the benchmarks showing that this is faster for the cases we care about without slowing down our other benchmark cases.

@segiddins
Copy link
Member Author

OK, will ship master as a 0.3.1, and if I get a 👍 on this, will add some changelog entries and make this 0.4.0.

I'm actually very happy with these changes, as it makes the dependency graph way simpler conceptually

@indirect
Copy link

👍 from me.

@alloy
Copy link
Member

alloy commented Jul 24, 2015

It looks good to me 👍 Would be nice if you could include some before/after numbers in the CHANGELOG.

@segiddins
Copy link
Member Author

@alloy the numbers are basically the same in all of the benchmarks I have, but it sped up that linked to issue by 25x

@alloy
Copy link
Member

alloy commented Jul 24, 2015

That seems like a very impressive number to include imo :)

segiddins added a commit that referenced this pull request Jul 26, 2015
@segiddins segiddins merged commit ef4e927 into master Jul 26, 2015
@segiddins segiddins deleted the seg-refactor branch July 26, 2015 07:02
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