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

Introduce singletone Locale to use identity hash maps #631

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

snuyanzin
Copy link
Collaborator

This could bring some perf improvements

@codecov-commenter
Copy link

Codecov Report

Merging #631 (a709b2e) into main (15a2d88) will decrease coverage by 0.25%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##               main     #631      +/-   ##
============================================
- Coverage     92.81%   92.56%   -0.25%     
- Complexity     2615     2618       +3     
============================================
  Files           280      281       +1     
  Lines          5371     5396      +25     
  Branches        587      589       +2     
============================================
+ Hits           4985     4995      +10     
- Misses          238      245       +7     
- Partials        148      156       +8     
Impacted Files Coverage Δ
...in/java/net/datafaker/internal/helper/FLocale.java 75.00% <75.00%> (ø)
.../java/net/datafaker/service/FakeValuesService.java 84.35% <81.81%> (-0.57%) ⬇️
...rc/main/java/net/datafaker/service/FakeValues.java 83.33% <83.33%> (+0.14%) ⬆️
.../main/java/net/datafaker/service/FakerContext.java 85.07% <84.37%> (+0.69%) ⬆️
.../main/java/net/datafaker/providers/base/Lorem.java 93.75% <0.00%> (-6.25%) ⬇️
...ker/idnumbers/pt/br/IdNumberGeneratorPtBrUtil.java 92.59% <0.00%> (-3.71%) ⬇️
.../datafaker/transformations/sql/SqlTransformer.java 87.45% <0.00%> (-0.74%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bodiam
Copy link
Contributor

bodiam commented Jan 17, 2023

But what is an FLocale?

@snuyanzin
Copy link
Collaborator Author

snuyanzin commented Jan 17, 2023

The problem with java's Locale is that it is not singletone and can not be used in IdentityHashMap where no need to calculate hash for keys and only equals is enough.

FLocale is a way of having such singletones per Locale. It guarantees that within one jvm/classloader there is only one instance of FLocale for java's Locale. It allows to use IdentityHashMap.
Finally this trick allows to save time because there is no need to calculate hashcode for Locale keys.

But what is an FLocale?

Should be just internal class. Normally users should not use it... Only in some specific cases probably

@bodiam
Copy link
Contributor

bodiam commented Jan 17, 2023

I'm not sure I follow. I don't think IdentityHashMap is using hashcode nor equals at all, it's using object references afaik.

Still don't really see why it's called FLocale though. F stands for Singleton?

@snuyanzin
Copy link
Collaborator Author

I don't think IdentityHashMap is using hashcode nor equals at all, it's using object references afaik.
yes you're right

Right now F stands for Faker.
Do you think SLocale (S for Singleton) would be better?

@bodiam
Copy link
Contributor

bodiam commented Jan 17, 2023

I'm not a big fan of abbreviations where I have to guess what it means, so perhaps SingletonLocale might be a name, or otherwise just a line of Javadoc could clear things up?

@snuyanzin
Copy link
Collaborator Author

makes sense
renamed to SingletonLocale

@snuyanzin snuyanzin changed the title Introduce singletone FLocale to use identity hash maps Introduce singletone Locale to use identity hash maps Jan 17, 2023
@bodiam
Copy link
Contributor

bodiam commented Jan 18, 2023

I still don't really understand why there's a hashcode and and an equals in the SingletonLocale if you're only going to use it in an IdentityHashmap. Why did you need those?

@what-the-diff
Copy link

what-the-diff bot commented Jan 18, 2023

  • SingletonLocale is introduced to use IdentityHashMap for locales.
  • FakeValues class was refactored and now uses SingletonLocale instead of Locale directly.
  • FakerContext#getSingletonLocale() method was added, which returns a singleton locale instance based on the current context's locale chain (the first one).
  • The following methods were removed from FakerContext: getDefaultFakeValue(), setDefaultFakeValue(). They are not used anywhere in the codebase anymore after Bump junit from 4.12 to 4.13.1 #1-#3 changes above have been applied; also they don't make sense as there can be multiple fake values instances per each context depending on its locale chain length (#locales > 1) or if custom paths/files were provided via addPath(...) calls before calling any faking API methods like fetchObject(...), resolve(...), etc..
  • The class FakerContext was changed.
  • A new import statement for the SingletonLocale helper class was added to this file, and a few other imports were removed as they are no longer needed (HashMap).

@snuyanzin
Copy link
Collaborator Author

I still don't really understand why there's a hashcode and and an equals in the SingletonLocale if you're only going to use it in an IdentityHashmap. Why did you need those?

There are 2 cases

  1. IdentityHashMap and for this one there is no need for hashCode, equals.
  2. SingletonLocale is a field inside net.datafaker.service.FakerContext and there are hash maps where FakerContext is a key, e.g. net.datafaker.providers.base.BaseFaker#CLASSES. For this keys we need to calculate hashcodes as part of calculation of hashcode for net.datafaker.service.FakerContext

@bodiam
Copy link
Contributor

bodiam commented Jan 18, 2023

Ah, I missed that one. Thanks for taking the time to explain!

@snuyanzin
Copy link
Collaborator Author

no problem
good that now it seems we are on the same page

@snuyanzin snuyanzin merged commit 53e14a4 into datafaker-net:main Jan 18, 2023
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