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.

Sign up for GitHub

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

Jump to bottom

Replace poly tables #2462

Merged
merged 16 commits into from
Jul 29, 2019
Merged

Replace poly tables #2462

merged 16 commits into from
Jul 29, 2019

Conversation

rgrinberg
Copy link
Member

Table is superior to it and just as convenient. This required me to fill out the rest of the implementation of Table.

@rgrinberg rgrinberg requested review from a user and aalekseyev July 27, 2019 10:11
@rgrinberg rgrinberg requested a review from emillon as a code owner July 27, 2019 10:11
aalekseyev
aalekseyev approved these changes Jul 29, 2019
View reviewed changes
Copy link
Collaborator

@aalekseyev aalekseyev left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!
By the way, I have to say that I never tested the performance impact of that functor. I'll run some benchmark.

src/stdune/hashtbl.ml Show resolved Hide resolved
@rgrinberg
Copy link
Member Author

By the way, I have to say that I never tested the performance impact of that functor. I'll run some benchmark.

Isn't it how it works in base?

rgrinberg added 15 commits July 29, 2019 18:41
@rgrinberg
[refactor] Hashtbl.t with Table.t in Lib.t
b7db814 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace polymorphic hashtable in dune_project
f18cd8c 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace polymorphic hashtable in alias
17c29fd 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace polymorphic table in gen_rules
bf8b6f6 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] Replace polymorphic table in versioned_file
11e47d5 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace poly hashtable in opam_file
fa5542f 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace polymorphic hashtbl in preprocessing
a80208f 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace poly table in response_file
e2767c2 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] poly table removal in findlib
a6e72d1 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace poly tables in context
5f4b50d 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] remove polymorphic table in scheduler
03e8312 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] replace poly table in super_context
0326953 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] remove poly table from build_system
621f8e0 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[stdune] Do not expose polymorphic hashtbl functions
9cadb8d 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
[refactor] remove poly table from interned
39e3de1 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg force-pushed the replace-poly-tables branch from b1e422b to 39e3de1 Compare July 29, 2019 11:43
@aalekseyev
Copy link
Collaborator

No, in base the implementation is completely different (it's a different data structure from stdlib one)

@aalekseyev
Copy link
Collaborator

I ran dune build --profile=release --debug-dependency-path @cli || true in ocaml-platform at the base and the tip of this PR 250 times each.

Saw nothing statistically significant and the effect is clearly small.

base avg.: 2.0333s
tip avg.: 2.0454s

95% confidence interval of difference: (-0.01 .. +0.034)

@rgrinberg
Hashtbl.to_dyn: sort hashtable keys
0cfad4e 
Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg rgrinberg merged commit 799524c into ocaml:master Jul 29, 2019
@rgrinberg rgrinberg deleted the replace-poly-tables branch July 29, 2019 13:33
@ghost
Copy link

ghost commented Jul 30, 2019

Thanks @aalekseyev for benching this! It's important to get an idea of the impact of such changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalekseyev aalekseyev aalekseyev approved these changes

@emillon emillon Awaiting requested review from emillon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @aalekseyev