Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Store list of generators with enabled uniqueness for faster clear #1246

Merged
merged 4 commits into from
Jul 19, 2018
Merged

Store list of generators with enabled uniqueness for faster clear #1246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5beda2
Store list of generators with enabled uniqueness for faster clear
MarcPer May 22, 2018
bc9c38d
Update CHANGELOG.md
vbrazo Jul 19, 2018
6df1ec5
Update CHANGELOG.md
vbrazo Jul 19, 2018
1e39cf6
Merge branch 'master' into speed_up_unique_clear
vbrazo Jul 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
- [PR #1115](https://github.com/stympy/faker/pull/1115) Add Faker::Cosmere [@JauntyJames](https://github.com/JauntyJames)
- [PR #801](https://github.com/stympy/faker/pull/801) Add Faker::NHS - Support for the British National Health Service [@substrakt-health](https://github.com/substrakt-health)

### Suggestion
- [PR #1246](https://github.com/stympy/faker/pull/1246) Store list of generators with enabled uniqueness for faster clear [@MarcPer](https://github.com/MarcPer)

------------------------------------------------------------------------------
## [v1.9.1](https://github.com/stympy/faker/tree/v1.9.1) (2018-07-11)
[Full Changelog](https://github.com/stympy/faker/compare/v1.8.7...v1.9.1)
Expand Down
10 changes: 9 additions & 1 deletion lib/helpers/unique_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,15 @@

module Faker
class UniqueGenerator
@marked_unique = Set.new # Holds names of generators with unique values

class << self
attr_reader :marked_unique
end

def initialize(generator, max_retries)
@generator = generator
self.class.marked_unique.add(self)
@max_retries = max_retries
@previous_results = Hash.new { |hash, key| hash[key] = Set.new }
end
Expand Down Expand Up @@ -34,7 +41,8 @@ def clear
end
Copy link

@maschwenk maschwenk Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason we can't just reassign

def clear
  @previous_results = Hash.new { |hash, key| hash[key] = Set.new }
end

That passes all specs and seems to be fast. Is the fear that there would be a big pressure on the GC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this faster than @previous_resuls.clear?

Copy link

Choose a reason for hiding this comment

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

I'd need to do a proper benchmark, but I noticed this before I started using your new code here which seems plenty fast. Was purely curious what the reasoning was. Thanks so much for this PR btw! Can't wait till it's released because right now i can't use the old implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! I'm actually curious about this as well. If you do the benchmarks, please tell us about it.


def self.clear
ObjectSpace.each_object(self, &:clear)
marked_unique.each(&:clear)
marked_unique.clear
end

def exclude(name, arguments, values)
Expand Down