-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
new LinkedHashMap
/LinkedHashSet
implementation
#10221
Conversation
41ada93
to
7b5d178
Compare
All tests passed. So I have run some benchmark testing to get some performance data. I copied mutable/HashMapBenchmark2.scala and made the linkedhashmap/linkedhashset version. Please give comment to improve the benchmark program if needed.
|
7b5d178
to
c4402d2
Compare
The linkedhashset benchmarking data is as below:
|
2c746fc
to
af64e30
Compare
Thank you @liang3zy22! I started reviewing this PR, it's looking good. I haven't finished yet though. I pushed two commits to the branch of this PR. From looking at the numbers you posted, the performance improvements are really good. As far as I can tell, there are optimizations in
The PR currently overrides the same operation that the old Serialization ( |
Thanks for review, @lrytz ! Yes, I only overrides same operation as old LinkedHashMap for binary compatibility. |
152ae6b
to
a238683
Compare
I finished looking at the code changes, it's looking good to me.
Thank you, I missed that dependency. I restored
When looking why |
(Let's run the community build on this: after we think it's in final form, but before actual merge.) |
Yeah, don't know how scala serialization work then written unused codes(just know that writeReplace function needed to be overridden). |
community build run on 2.13.11-bin-89f3de5-SNAPSHOT: https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4149/ also, @lrytz when we spoke today you mentioned doing some rebasing here, but I'm not seeing a need for it? your individual commits seem coherent? |
OK, we can leave the commits as they are. Or squash the first two. @SethTisue maybe you can take a look at the community build log, it's not clear to me what to make of it. |
The mutable HashMap/HashSet has been rewroten and the performance is better. So rewrote the LinkedHashMap and LinkedHashSet also to improve performance. The detailed data can be seen in the PR. Most codes are same with HashMap/HashSet but some are different: 1. To keep binary compatibility, only api in old solution are updated.The two class LinkedHashMap/LinkedHashSet still don't have parameters. hashcode can't be realized since it needs a new iterator which will break binary compatibility. 2. Add specific method to handle the order when adding/removing the entry. 3. other minor changes. Signed-off-by: Liang Yan <ckgppl_yan@sina.cn>
89f3de5
to
5d1dd06
Compare
I squashed those two commits. Community build is green at https://scala-ci.typesafe.com/job/scala-2.13.x-jdk11-integrate-community-build/4151/ (the metals failure is known to happen with Scala |
* Add `LinkedHashSet.add` override * Mark the private[collection] HashTable as not used
`LinkedHashMap/Set` extend `DefaultSerializable`, so instances are not directly serialized, the `DefaultSerializationProxy` is used instead. Serialization-related code in `LinkedHashMap/Set` is unused.
5d1dd06
to
d07b344
Compare
In honor of Irene Cara, someone should play "What a Feeling!" 🎉 |
It's the first time I heard about this song, I can't do it. |
LinkedHashMap
/LinkedHashSet
implementation
This PR is to fix scala/bug#11369. The old PR is closed, so create new one. The benchmark data will be added if all tests pass.