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

Remove various helper functions from Action_builder modules #9132

Closed
wants to merge 10 commits into from

Conversation

snowleopard
Copy link
Collaborator

@rgrinberg As discussed, I'm cleaning up the implementation of Action_builder modules, so I'm deleting all inessential helpers that can be easily implemented on the rules side.

Could you take over from here, re-adding the helpers in Dune rules? It's probably easiest to extend the Action_builder module when importing Dune_engine in the rules by just copying the helpers I deleted.

@snowleopard snowleopard requested a review from rgrinberg November 9, 2023 22:58
@snowleopard snowleopard force-pushed the clean-up-action-builder branch from d93a5d2 to 2cc50a6 Compare November 9, 2023 23:02
Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard snowleopard force-pushed the clean-up-action-builder branch from 2cc50a6 to ef9f8a5 Compare November 9, 2023 23:05
@snowleopard
Copy link
Collaborator Author

snowleopard commented Nov 9, 2023

Btw, this clean up includes simplifying the implementation of paths_matching (previously called paths_matching_unit) by noticing that the result of the computation is just ignored. With all the helpers code around, it was not obvious that the code could be made simpler. Now it is.

Internal benchmarks show consistent improvements in build times (1-2%) and allocations (up to 1%).

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard
Copy link
Collaborator Author

Actually, I've just realised that Action_builder.path_matching was just a trivial special case of Action_builder.record, so I nuked it too.

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard
Copy link
Collaborator Author

snowleopard commented Nov 12, 2023

I've removed Action_builder.contents path which can now be replaced with Action_builder.of_memo (Memo.of_thunk_apply Build_system.read_file path), which looks like a little performance improvement in my internal benchmarks.

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard
Copy link
Collaborator Author

snowleopard commented Nov 13, 2023

Action_builder.*file_exists are gone too. They can be trivially implemented via Build_system.file_exists.

I noticed that internally we had a bunch of incorrect uses of Action_builder.memo where we passed a prematurely forced computation, so I expanded the comment for Action_builder.memo.

@snowleopard snowleopard force-pushed the clean-up-action-builder branch from dc21fba to 02f22ba Compare November 13, 2023 01:18
Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard snowleopard force-pushed the clean-up-action-builder branch from 02f22ba to 6e1b343 Compare November 13, 2023 13:24
@rgrinberg
Copy link
Member

@snowleopard do you have more changes pending or should I update the rules?

@snowleopard
Copy link
Collaborator Author

@snowleopard do you have more changes pending or should I update the rules?

There are a couple of more removals I'm working on. I'll ping you in the next couple of days!

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard snowleopard force-pushed the clean-up-action-builder branch from 04e2ebc to 6492d1f Compare November 15, 2023 22:32
Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard
Copy link
Collaborator Author

@rgrinberg OK, I think I'm done. My next step is changing the implementation of Action_builder.t but that will be done in a separate PR. Please take over from here, and let me know if I messed up anything.

@rgrinberg
Copy link
Member

@snowleopard I think you forgot to push Dep.Facts.record_facts and the visibility_override in dune_engine/action.ml

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard
Copy link
Collaborator Author

snowleopard commented Nov 16, 2023

I think you forgot to push Dep.Facts.record_facts and the visibility_override in dune_engine/action.ml

@rgrinberg Oops, thanks. visibility_override is just an internal thing, so I removed it.

I added Dep.Facts.record_facts, though a few other changes happened in dep.ml so I won't be surprised if I'm missing something else... Let me know!

@rgrinberg
Copy link
Member

Completed in a series of PR's culminating in #9559

@rgrinberg rgrinberg closed this Dec 21, 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.

2 participants