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

Clean up code conversions of Profile enum #1511

Merged
merged 1 commit into from Jan 19, 2023

Conversation

GuillaumeGomez
Copy link
Member

This cleans up a bit some conversion of the Profile type and allow to have it in one place.

Another thing I wondered: this type is also present in both collector/src/benchmark/profile.rs and database/src/lib.rs. Is there a reason why it is duplicated like this? collector/src/benchmark/ has database as a dependency, so is there a particular reason why Profile was also created in collector/src/benchmark? If not I'm planning to merge both types in a follow-up PR.

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Before the recent refactoring, the collector Profile was slightly different from the database Profile, but after the refactoring their "layout" is indeed the same.

That being said, I'm not sure if it's needed to unify them just because they have the same layout/attributes. The collector uses the profile/scenario enums in a slightly different way than the database (it's subtle, but it can be seen by the different methods and derived trait implementations). I consider the database enums to be "implementation details" of how is this information stored in the database, which should be able to be changed independently of the collector's way of representing profiles (and in theory it's possible that this will change once we will change the DB layout to accommodate runtime benchmarks). And since the conversion between them is trivial, I think it's fine if it stays the way it is.

@GuillaumeGomez
Copy link
Member Author

As you prefer. I wondered about it because I'm currently working on something that needed to add a new entry into Profile and I basically had to do it twice, so not great.

@Kobzol
Copy link
Contributor

Kobzol commented Jan 10, 2023

I see. I still think that it's safer to just add the thing twice rather than to unify it (the scenario enums are also not unified, which is an example of "the same enum", which has different representations in DB and in collector).

What are you working on? :)

@GuillaumeGomez
Copy link
Member Author

To answer your question: #1512 (this is very much open to debate and not ready at all, just wanted to see if it was worth it before going any further).

@GuillaumeGomez
Copy link
Member Author

Is there anything else to be done here @Kobzol ?

@Kobzol Kobzol merged commit 3af3ba1 into rust-lang:master Jan 19, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Jan 19, 2023

Sorry for the delay, merged.

@GuillaumeGomez GuillaumeGomez deleted the cleanup-profile-conversions branch January 19, 2023 13:24
@GuillaumeGomez
Copy link
Member Author

Thanks!

@nnethercote
Copy link
Contributor

That being said, I'm not sure if it's needed to unify them just because they have the same layout/attributes. The collector uses the profile/scenario enums in a slightly different way than the database (it's subtle, but it can be seen by the different methods and derived trait implementations). I consider the database enums to be "implementation details" of how is this information stored in the database, which should be able to be changed independently of the collector's way of representing profiles (and in theory it's possible that this will change once we will change the DB layout to accommodate runtime benchmarks).

This would be great information to put in a comment on each Profile definition!

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.

None yet

3 participants