coconut-svsm / svsm

COCONUT-SVSM
MIT License
120 stars 40 forks source link

Enable Link-Time Optimization (LTO) #488

Open zamazan4ik opened 1 week ago

zamazan4ik commented 1 week ago

Hi!

I noticed that in the Cargo.toml file Link-Time Optimization (LTO) for the project is not enabled. I suggest switching it on since it will reduce the binary size (always a good thing to have) and will likely improve the application's performance a bit.

I suggest enabling LTO only for the Release builds so as not to sacrifice the developers' experience while working on the project since LTO consumes an additional amount of time to finish the compilation routine. If you think that a regular Release build should not be affected by such a change as well, then I suggest adding an additional dist or release-lto profile where additionally to regular release optimizations LTO will also be added. Such a change simplifies life for maintainers and others interested in the project persons who want to build the most performant version of the application. Using ThinLTO should also help to reduce the build-time overhead with LTO. If we enable it on the Cargo profile level, users, who install the application with cargo install, will get the LTO-optimized version "automatically". E.g., check cargo-outdated Release profile.

Basically, it can be enabled with the following lines:

[profile.release]
lto = true

Thank you.

Freax13 commented 1 week ago

Why are you spamming?

image image image

Have you considered opening an issue in cargo instead of opening issues in seemingly random GitHub repos?

Quite frankly, your comments here and in https://github.com/coconut-svsm/svsm/discussions/489 show that you're not familiar with the project at all and some of them are completely inapplicable.

I'm sorry if I'm coming off unusually hostile, but I really don't like spam even if the thing that's being spammed seems somewhat reasonable at face value. If the goal is to improve the performance of all Rust projects, there are better ways of going about that, this is clearly farming for engagements/stats/whatever.

zamazan4ik commented 1 week ago

Why are you spamming? Have you considered opening an issue in cargo instead of opening issues in seemingly random GitHub repos?

Yes, I of course considered opening an issue directly in Cargo about enabling LTO by default for Cargo projects - e.g. this https://github.com/rust-lang/cargo/issues/11298 (plus there was several discussions about the same thing in the Rust Zulip).

The problem is that Cargo devs (the Rust team) nowadays hesitate to enable LTO by default mainly because they are afraid of increasing the compile time for all projects - it can affect build times/memory consumption for Rust users too much.

That's why I chose another way - creating/promoting LTO over all existing Rust ecosystem (about PGO I do the same thing btw - you also can see it via GitHub stats), gathering statistics about how many projects enable LTO after my request (usually they just enable it - you can also check it via GitHub issues), and show this stat to the Rust dev team once again as an additional proof for enabling LTO by default for the Rust ecosystem.

Quite frankly, your comments here and in https://github.com/coconut-svsm/svsm/discussions/489 show that you're not familiar with the project at all and some of them are completely inapplicable.

Even if I am not familiar with this project, I still can suggest improvements to the project since it's published on GitHub. Regarding the required expertise to suggest LTO/PGO to the project:

So I don't think that I need a deep understanding of SVSM internals to suggest optimizations like LTO and PGO.

this is clearly farming for engagements/stats/whatever.

Nope, it isn't. The only idea behind creating these issues is to improve the performance baseline for the whole Rust ecosystem by using more aggressive compiler optimizations by default. I don't care at all about star numbers, contributor badges, commit/PR/issues count, etc. - you can trust me or not - it's completely up to you. And I will continue my work in this area even if you think it's spam (FMPOV it isn't).

If the goal is to improve the performance of all Rust projects, there are better ways of going about that

I would be happy to discuss with you this better ways for improving the whole Rust ecosystem. If you have ideas - feel free to share them. Maybe I just missed something basic and I would be happy to use in the future.

Regarding LTO and PGO for SVSM - if you a part of the development team, feel free just close both issues if you don't care much about them for the project - it's completely fine! Of course, it's up to the project devs to choose optimization features for their project.

P.S. I have been doing this job for more than 2 years across maaaaaaany projects. Some stats - I created more than 500 issues about PGO (about LTO a bit less - but it's only for now). And you are the first (or the second at most) person who complained about it.

Freax13 commented 1 week ago

Why are you spamming? Have you considered opening an issue in cargo instead of opening issues in seemingly random GitHub repos?

Yes, I of course considered opening an issue directly in Cargo about enabling LTO by default for Cargo projects - e.g. this rust-lang/cargo#11298 (plus there was several discussions about the same thing in the Rust Zulip).

You didn't create this issue nor did you even comment on it or help bring it forward in any way (at least in any way that's immediately obvious to me). If this is something important to you and something that you care about, I suggest contributing to cargo directly. What you're doing here isn't solving the problem, at least not in a way that scales.

The problem is that Cargo devs (the Rust team) nowadays hesitate to enable LTO by default mainly because they are afraid of increasing the compile time for all projects - it can affect build times/memory consumption for Rust users too much.

I'm sorry, but I'm not sure that's a good argument. If there are valid concerns about enabling LTO everywhere, then I don't see how picking completely random projects and telling all of them to enable LTO is any better. If there are concerns with enabling LTO, you're bound to eventually open issues on projects that would be affected by those concerns.

That's why I chose another way - creating/promoting LTO over all existing Rust ecosystem (about PGO I do the same thing btw - you also can see it via GitHub stats), gathering statistics about how many projects enable LTO after my request (usually they just enable it - you can also check it via GitHub issues), and show this stat to the Rust dev team once again as an additional proof for enabling LTO by default for the Rust ecosystem.

I also see a lot of issues that never get any activity. Have you made any efforts so far to show any of those stats to the Rust dev team? You say you've been doing this for 2 years now, surely you must have talked to them by now.

Quite frankly, your comments here and in #489 show that you're not familiar with the project at all and some of them are completely inapplicable.

Even if I am not familiar with this project, I still can suggest improvements to the project since it's published on GitHub. Regarding the required expertise to suggest LTO/PGO to the project:

  • I see a lot of dependencies in the project. It means that LTO can help with optimizing the project size at least and likely improve CPU performance since enabling LTO will allow performing cross-crate dependencies optimization.

You don't need to be an expert to know that projects with many dependencies benefit from LTO. That said, have you actually counted the dependencies? Really, have you? I count 29 with more than half of that being pulled by depending on aes-gcm. For a Rust project, 29 dependencies is not a lot by far, especially if you consider that most of them here are very small and make heavy use of generics that can be inlined across crate boundaries anyways. I've worked on projects with 800+ deps, trust me, 29 is barely anything and to suggest that it's a lot once again makes me think that you didn't actually look that closely at the project.

  • I see quite a large amount of code in the kernel part of the project. I've seen similar projects (like CrosVM and other kernels like Linux kernel) that have quite similar code inside (semantically), and they benefit from enabling PGO (according to the available benchmarks). That's why I decided that PGO is applicable to the project too.

I'm sorry, but I don't think this is helping very much. Telling us to use PGO, is like looking at a fighter jet and telling your friend who's into model airplanes to build a turbofan for their plane. It's cool that using PGO in kernels works for others, but I'm guessing it's a hell of a lot of work and it certainly isn't a priority at all right now. You may think that PGO is applicable because it can be implemented for kernels, but that once again completely ignores the context of this project.

Heck, while we're at it, neither performance nor binary size are really a primary concern right now, our main priority right now is mainly to get things working.

Also, CrosVM isn't a kernel, it's a VMM based on KVM. Do you start to see why I think you're valuing quantity over quality?

Another thing is that reproducibility is really important for this project. Did you consider this? That's not a rhetorical question, I want an answer to this. Did you consider this? Your own repo lists reproducibility as a potential blocker for PGO. If you did consider that we need reproducibility, why didn't you mention anything about that? Sure, there may be ways to work around reproducibility issues, but once again, that's work I don't think we don't have the capacities for, so, again, PGO maybe just isn't a good fit here.

So I don't think that I need a deep understanding of SVSM internals to suggest optimizations like LTO and PGO.

And I disagree for the reasons listed above.

Here's another example of a suggestion that just straight-up doesn't apply: In #489 you mentioned that we could use cargo-pgo. cargo-pgo doesn't work in no_std environments. I don't blame you for not knowing that cargo-pgo won't work here. If I looked at a bunch of projects and had to make a bunch of suggestions, I'd make many mistakes too. My point is that just looking at the surface isn't enough to make suggestions and think that that's making an informed decision.

this is clearly farming for engagements/stats/whatever.

Nope, it isn't. The only idea behind creating these issues is to improve the performance baseline for the whole Rust ecosystem by using more aggressive compiler optimizations by default. I don't care at all about star numbers, contributor badges, commit/PR/issues count, etc. - you can trust me or not - it's completely up to you. And I will continue my work in this area even if you think it's spam (FMPOV it isn't).

FWIW I consider "gathering statistics about how many projects enable LTO after my request" (your words) to be stats farming. Just by how the way the statement is phrased, it sounds to me like you're spamming out random issues without much thought and looking at the results. It's certainly a way to get the stats, but let's not pretend it's not spam.

If the goal is to improve the performance of all Rust projects, there are better ways of going about that

I would be happy to discuss with you this better ways for improving the whole Rust ecosystem. If you have ideas - feel free to share them. Maybe I just missed something basic and I would be happy to use in the future.

Contribute directly to cargo/Rust. Seriously. If you think the current defaults are problematic any impact you make there will be much much bigger than any impact you have by asking individual projects to enable LTO/PGO. We need people working on cargo and/or Rust. Better yet, maybe work to resolve the concerns people have with always enabling LTO/PGO.

Regarding LTO and PGO for SVSM - if you a part of the development team, feel free just close both issues if you don't care much about them for the project - it's completely fine! Of course, it's up to the project devs to choose optimization features for their project.

I'm not a maintainer for this project, nor do I want to claim to be one, so that decision is not on me. That said I care about this project and I deeply care about the culture on GitHub.

P.S. I have been doing this job for more than 2 years across maaaaaaany projects. Some stats - I created more than 500 issues about PGO (about LTO a bit less - but it's only for now). And you are the first (or the second at most) person who complained about it.

:1st_place_medal:

FWIW you're not the first person I encountered sending out mass suggestions without considering the project's context. I maintain a few other projects (again, I'm not a maintainer of this project, but I do other stuff as well) and had multiple suggestions like yours come in. We ended up rejecting at least some of the suggestions. What I'm asking you is to consider our side of this as well. Sending out stuff is incredibly easy, but dealing with it isn't always as easy and has the potential to waste resources that are better spent on other work.

Please consider contributing to cargo and/or Rust.

zamazan4ik commented 1 week ago

You didn't create this issue nor did you even comment on it or help bring it forward in any way (at least in any way that's immediately obvious to me). If this is something important to you and something that you care about, I suggest contributing to cargo directly. What you're doing here isn't solving the problem, at least not in a way that scales.

You didn't see my private conversation about this topic with Rust devs in Discord. Believe me or not - I discussed this topic and got the same answer as I mentioned earlier - "by default harming build times is too dangerous".

I'm sorry, but I'm not sure that's a good argument. If there are valid concerns about enabling LTO everywhere, then I don't see how picking completely random projects and telling all of them to enable LTO is any better. If there are concerns with enabling LTO, you're bound to eventually open issues on projects that would be affected by those concerns.

My point is that increasing by default build times (the usual consequence from enabling LTO - mostly Fat version since Thin has far less build time overhead) is not a concern for a majority of projects in the ecosystem. That's why even if Cargo doesn't want to enable LTO by default due to concerns, it's still a non-issue for most of the projects.

FWIW I consider "gathering statistics about how many projects enable LTO after my request" (your words) to be stats farming. Just by how the way the statement is phrased, it sounds to me like you're spamming out random issues without much thought and looking at the results. It's certainly a way to get the stats, but let's not pretend it's not spam.

I care about resulting results in projects too. E.g. I did many performance benchmarks for PGO requests (not for all, but for many of them) - the same is true for LTO: some of them contain statistics about the binary size reduction that I actually measured on my dev machine. And yes - I care about binary sizes for the Rust ecosystem.

You are free to mark my issues as spam - I don't care much tbh - it's not my first time in the Internet :) I just know that it isn't. I am happy that many projects enabled LTO for their builds so the Rust ecosystem became a bit more optimized by default. PGO is a trickier part of enablement for multiple reasons but some projects in one or another way integrated PGO after my reports - I am happy about that too.

I also see a lot of issues that never get any activity. Have you made any efforts so far to show any of those stats to the Rust dev team? You say you've been doing this for 2 years now, surely you must have talked to them by now.

IMO, even one issue about integrating PGO/LTO into a project is a win. And I have much more of them. That's just a personal opinion as yours: you think it's not worth it, I think otherwise.

Here's another example of a suggestion that just straight-up doesn't apply: In https://github.com/coconut-svsm/svsm/discussions/489 you mentioned that we could use cargo-pgo. cargo-pgo doesn't work in no_std environments. I don't blame you for not knowing that cargo-pgo won't work here. If I looked at a bunch of projects and had to make a bunch of suggestions, I'd make many mistakes too. My point is that just looking at the surface isn't enough to make suggestions and think that that's making an informed decision.

Fortunately, I know how cargo-pgo works. I specifically wrote "trying to start" as "start to play with PGO" to get some experience with it. Of course, it's not suitable for many envs (including yours, as you mentioned above). By the way, no_std limitation is a good and valid reason to at least investigate other ways to implement PGO (e.g. via Sampling PGO that doesn't require instrumentation and should work here too - just my guess here).

I am sure that my suggestion for SVSM is still valid in both LTO and PGO cases.

I'm sorry, but I don't think this is helping very much. Telling us to use PGO, is like looking at a fighter jet and telling your friend who's into model airplanes to build a turbofan for their plane. It's cool that using PGO in kernels works for others, but I'm guessing it's a hell of a lot of work and it certainly isn't a priority at all right now. You may think that PGO is applicable because it can be implemented for kernels, but that once again completely ignores the context of this project. Heck, while we're at it, neither performance nor binary size are really a primary concern right now, our main priority right now is mainly to get things working.

That's a valid argument! I just didn't find information in the README file that this project doesn't care about performance and the binary size. If it's not a project priority - just say it and close my issue, that's it. I can fully understand such an argument and I am completely ok with it since it's a sane argument.

Another thing is that reproducibility is really important for this project. Did you consider this? That's not a rhetorical question, I want an answer to this. Did you consider this? Your own repo lists reproducibility as a potential blocker for PGO. If you did consider that we need reproducibility, why didn't you mention anything about that? Sure, there may be ways to work around reproducibility issues, but once again, that's work I don't think we don't have the capacity for, so, again, PGO maybe just isn't a good fit here.

I am an oracle - I don't know which concerns are valid for which projects. If I will mention ALL actual and potential issues with PGO, I will need to copy like a third of my article :) (just a phrase, pls don't try to nitpick it). If SVSM devs decide that reproducibility is important for the project and they are not okay with the suggested in the article way to achieve it (e.g. via committing PGO profiles to the repo) - well, it's another good technical point to not integrate PGO into the project. I am fine with it too.

You don't need to be an expert to know that projects with many dependencies benefit from LTO. That said, have you actually counted the dependencies? Really, have you? I count 29 with more than half of that being pulled by depending on aes-gcm. For a Rust project, 29 dependencies is not a lot by far, especially if you consider that most of them here are very small and make heavy use of generics that can be inlined across crate boundaries anyways. I've worked on projects with 800+ deps, trust me, 29 is barely anything and to suggest that it's a lot once again makes me think that you didn't actually look that closely at the project.

I disagree with this point of view. For me, even one dependency is enough to enable LTO since not-enabling it is a missing optimization opportunity. And yes, I counted them - 29 dependencies FMPOV is a valid argument to enable LTO even for the Rust ecosystem.

FWIW you're not the first person I encountered sending out mass suggestions without considering the project's context. I maintain a few other projects (again, I'm not a maintainer of this project, but I do other stuff as well) and had multiple suggestions like yours come in. We ended up rejecting at least some of the suggestions. What I'm asking you is to consider our side of this as well. Sending out stuff is incredibly easy, but dealing with it isn't always as easy and has the potential to waste resources that are better spent on other work.

It's up to the project to reject suggested ideas from the community. If SVSM devs want to do it - I am completely ok with it (if they are rejected due to technical reasons not "political" (don't know which word is better here).

Contribute directly to cargo/Rust. Seriously. If you think the current defaults are problematic any impact you make there will be much much bigger than any impact you have by asking individual projects to enable LTO/PGO. We need people working on cargo and/or Rust. Better yet, maybe work to resolve the concerns people have with always enabling LTO/PGO. Please consider contributing to cargo and/or Rust.

I considered it - and I do it in my own way. Is it efficient or not - it's up to me (I think it is).

Freax13 commented 1 week ago

You didn't create this issue nor did you even comment on it or help bring it forward in any way (at least in any way that's immediately obvious to me). If this is something important to you and something that you care about, I suggest contributing to cargo directly. What you're doing here isn't solving the problem, at least not in a way that scales.

You didn't see my private conversation about this topic with Rust devs in Discord. Believe me or not - I discussed this topic and got the same answer as I mentioned earlier - "by default harming build times is too dangerous".

I don't consider private conversations with cargo devs to be contributing (who would?). If anything, if you talked to the cargo people and they told you that enabling LTO by default has the potential to do harm, maybe that's a reason not to be doing it. My issue was not with whether or not changing the default is justified, but whether or not you're meaningfully contributing. Please consider contributing to cargo by writing code, reviewing code, and engaging in discussions. Am I being crazy here? Surely, it should be obvious that making an impact in cargo will make a much larger impact on the broader ecosystem using it than changing every single project.

I'm sorry, but I'm not sure that's a good argument. If there are valid concerns about enabling LTO everywhere, then I don't see how picking completely random projects and telling all of them to enable LTO is any better. If there are concerns with enabling LTO, you're bound to eventually open issues on projects that would be affected by those concerns.

My point is that increasing by default build times (the usual consequence from enabling LTO - mostly Fat version since Thin has far less build time overhead) is not a concern for a majority of projects in the ecosystem. That's why even if Cargo doesn't want to enable LTO by default due to concerns, it's still a non-issue for most of the projects.

That completely misses the point. My point was that if there are reservations about changing the default, then it's maybe not a good idea to go around and ask everyone to change the default. It doesn't matter if it's thin or fat LTO. If the problem was only with fat LTO, cargo could just switch to thin LTO by default, but, again, they don't. It doesn't matter what the optimization is, if there are reservations against broadly applying it for every project, this should be best discussed with the cargo devs rather than in every single project using cargo.

FWIW I consider "gathering statistics about how many projects enable LTO after my request" (your words) to be stats farming. Just by how the way the statement is phrased, it sounds to me like you're spamming out random issues without much thought and looking at the results. It's certainly a way to get the stats, but let's not pretend it's not spam.

I care about resulting results in projects too. E.g. I did many performance benchmarks for PGO requests (not for all, but for many of them) - the same is true for LTO: some of them contain statistics about the binary size reduction that I actually measured on my dev machine. And yes - I care about binary sizes for the Rust ecosystem.

You are free to mark my issues as spam - I don't care much tbh - it's not my first time in the Internet :) I just know that it isn't. I am happy that many projects enabled LTO for their builds so the Rust ecosystem became a bit more optimized by default. PGO is a trickier part of enablement for multiple reasons but some projects in one or another way integrated PGO after my reports - I am happy about that too.

I'm not denying that you have made a positive impact for some projects.

I also see a lot of issues that never get any activity. Have you made any efforts so far to show any of those stats to the Rust dev team? You say you've been doing this for 2 years now, surely you must have talked to them by now.

IMO, even one issue about integrating PGO/LTO into a project is a win. And I have much more of them. That's just a personal opinion as yours: you think it's not worth it, I think otherwise.

Again, that misses my point. Allow me to exaggerate to make my point very clear: If you sent out 10,000 issues and wasted the time of 10,000 devs and just one of them accepted your suggestion, I think we can both agree that this would be bad. Now, obviously, your ratio is not this bad, it's a lot better than that (seems more like roughly 50% to me, but I could easily be way off on that, I didn't look much into that, I don't think exact numbers are necessary here, but feel free to chime in if you happen to have them), but my point is that somewhere there is a line where if you sent out too many issues and get too little back, this isn't helping and that time would perhaps be better spent doing other things. It's not just about the people you helped, it's also about the resources you wasted. My argument is that IMO you have crossed this threshold.

Here's another example of a suggestion that just straight-up doesn't apply: In #489 you mentioned that we could use cargo-pgo. cargo-pgo doesn't work in no_std environments. I don't blame you for not knowing that cargo-pgo won't work here. If I looked at a bunch of projects and had to make a bunch of suggestions, I'd make many mistakes too. My point is that just looking at the surface isn't enough to make suggestions and think that that's making an informed decision.

Fortunately, I know how cargo-pgo works. I specifically wrote "trying to start" as "start to play with PGO" to get some experience with it.

I don't buy that. The direct quote is "For the Rust projects, I suggest trying to start with cargo-pgo.". This phrasing specifically mentions the SVSM in "the Rust projects". To me this doesn't read at all as "use cargo-pgo to play around with PGO on some toy projects" and it reads much more as "for the SVSM, I suggest trying to start with cargo-pgo".

Of course, it's not suitable for many envs (including yours, as you mentioned above). By the way, no_std limitation is a good and valid reason to at least investigate other ways to implement PGO (e.g. via Sampling PGO that doesn't require instrumentation and should work here too - just my guess here).

I am sure that my suggestion for SVSM is still valid in both LTO and PGO cases.

There are some gains to be had by LTO, but my estimate would be that we'd gain less than the average Rust project. PGO is completely inappropriate given the state of the project. If we wanted to increase performance, there'd be a ton of other low-hanging fruit we should rather pursue than doing a bunch of work to get PGO to work.

I'm sorry, but I don't think this is helping very much. Telling us to use PGO, is like looking at a fighter jet and telling your friend who's into model airplanes to build a turbofan for their plane. It's cool that using PGO in kernels works for others, but I'm guessing it's a hell of a lot of work and it certainly isn't a priority at all right now. You may think that PGO is applicable because it can be implemented for kernels, but that once again completely ignores the context of this project. Heck, while we're at it, neither performance nor binary size are really a primary concern right now, our main priority right now is mainly to get things working.

That's a valid argument! I just didn't find information in the README file that this project doesn't care about performance and the binary size. If it's not a project priority - just say it and close my issue, that's it. I can fully understand such an argument and I am completely ok with it since it's a sane argument.

But that's my point. I feel like you don't look much at the project and just send out issues without much thought. We don't excuse Nigerian prince email scams by saying that people can just delete them if they aren't interested. Spam is spam. If you send out a lot of issues without much though, you're bound to cause some projects harm.

Another thing is that reproducibility is really important for this project. Did you consider this? That's not a rhetorical question, I want an answer to this. Did you consider this? Your own repo lists reproducibility as a potential blocker for PGO. If you did consider that we need reproducibility, why didn't you mention anything about that? Sure, there may be ways to work around reproducibility issues, but once again, that's work I don't think we don't have the capacity for, so, again, PGO maybe just isn't a good fit here.

I am an oracle - I don't know which concerns are valid for which projects. If I will mention ALL actual and potential issues with PGO, I will need to copy like a third of my article :) (just a phrase, pls don't try to nitpick it). If SVSM devs decide that reproducibility is important for the project and they are not okay with the suggested in the article way to achieve it (e.g. via committing PGO profiles to the repo) - well, it's another good technical point to not integrate PGO into the project. I am fine with it too.

Again, you don't seem to value the quality of the issues you open and seem entirely focused on quantity.

You don't need to be an expert to know that projects with many dependencies benefit from LTO. That said, have you actually counted the dependencies? Really, have you? I count 29 with more than half of that being pulled by depending on aes-gcm. For a Rust project, 29 dependencies is not a lot by far, especially if you consider that most of them here are very small and make heavy use of generics that can be inlined across crate boundaries anyways. I've worked on projects with 800+ deps, trust me, 29 is barely anything and to suggest that it's a lot once again makes me think that you didn't actually look that closely at the project.

I disagree with this point of view. For me, even one dependency is enough to enable LTO since not-enabling it is a missing optimization opportunity.

Again, if that's your view, take it up the cargo people. They should be the ones you should be convincing, not me. Frankly, so far, I don't really care, and I don't think I ever said here that we shouldn't enable LTO.

Also, your response is shifting the goalpost. You claimed that the SVSM was using many dependencies, I claimed that it wasn't. Whether or not 29 is enough to justify LTO was never in question, the question was about whether 29 dependencies is a lot. Given that you work with a lot of Rust-based projects, you should know that 29 is not considered to be a lot by any standard.

And yes, I counted them - 29 dependencies FMPOV is a valid argument to enable LTO even for the Rust ecosystem.

I don't believe you. Name the 29 dependencies. Until you correctly name the 29 dependencies, I don't believe another word out of your mouth.

FWIW you're not the first person I encountered sending out mass suggestions without considering the project's context. I maintain a few other projects (again, I'm not a maintainer of this project, but I do other stuff as well) and had multiple suggestions like yours come in. We ended up rejecting at least some of the suggestions. What I'm asking you is to consider our side of this as well. Sending out stuff is incredibly easy, but dealing with it isn't always as easy and has the potential to waste resources that are better spent on other work.

It's up to the project to reject suggested ideas from the community. If SVSM devs want to do it - I am completely ok with it (if they are rejected due to technical reasons not "political" (don't know which word is better here).

You're not a part of the community for the vast, vast majority of the projects you contribute these kinds of issues to. You have never before interacted with most of them, you don't use at least some of them (I highly doubt you were ever using this project or were planing on doing so in the future). If you're interested in becoming part of a community, that's great, but opening an issue and then immediately leaving, isn't the way to do it.

Contribute directly to cargo/Rust. Seriously. If you think the current defaults are problematic any impact you make there will be much much bigger than any impact you have by asking individual projects to enable LTO/PGO. We need people working on cargo and/or Rust. Better yet, maybe work to resolve the concerns people have with always enabling LTO/PGO. Please consider contributing to cargo and/or Rust.

I considered it - and I do it in my own way. Is it efficient or not - it's up to me (I think it is).

Allow me to clarify: Please consider contributing to Rust and cargo directly and not just primarily to ecosystems using it. Any impact you make in Rust and cargo will be much greater and will take away fewer resources from other projects.

zamazan4ik commented 1 week ago

I don't consider private conversations with cargo devs to be contributing (who would?). If anything, if you talked to the cargo people and they told you that enabling LTO by default has the potential to do harm, maybe that's a reason not to be doing it. My issue was not with whether or not changing the default is justified, but whether or not you're meaningfully contributing. Please consider contributing to cargo by writing code, reviewing code, and engaging in discussions. Am I being crazy here? Surely, it should be obvious that making an impact in cargo will make a much larger impact on the broader ecosystem using it than changing every single project.

No, you are not crazy at all! Your point is completely valid. Cargo is simply is not ready to enable it by default. That's why I chose another way - enable LTO for the ecosystem behind Cargo, and after that show Cargo devs an additional data point that many Rust projects can enable LTO safely by default.

I am totally fine with suggested by your way to contribute. I just say that it's not the best option right now. You think otherwise - I am fine with a different opinion.

That completely misses the point. My point was that if there are reservations about changing the default, then it's maybe not a good idea to go around and ask everyone to change the default. It doesn't matter if it's thin or fat LTO. If the problem was only with fat LTO, cargo could just switch to thin LTO by default, but, again, they don't. It doesn't matter what the optimization is, if there are reservations against broadly applying it for every project, this should be best discussed with the cargo devs rather than in every single project using cargo.

I get your point of view. Via enabling LTO in as many projects as possible I at the same time improve the default optimization level of these projects and show to the Cargo dev team more and more examples that enabling LTO by default is a viable option for the ecosystem. It's a long process to convince them to do it - I am fine with it.

After gathering enough data points I will surely will talk with the Cargo dev team publicly (I think via a forum post or smth like that).

I'm not denying that you have made a positive impact for some projects.

Thanks!

Again, that misses my point. Allow me to exaggerate to make my point very clear: If you sent out 10,000 issues and wasted the time of 10,000 devs and just one of them accepted your suggestion, I think we can both agree that this would be bad. Now, obviously, your ratio is not this bad, it's a lot better than that (seems more like roughly 50% to me, but I could easily be way off on that, I didn't look much into that, I don't think exact numbers are necessary here, but feel free to chime in if you happen to have them), but my point is that somewhere there is a line where if you sent out too many issues and get too little back, this isn't helping and that time would perhaps be better spent doing other things. It's not just about the people you helped, it's also about the resources you wasted. My argument is that IMO you have crossed this threshold.

Got your point of view. My opinion is different - I didn't cross any threshold at all in this situation. We just have different opinions on this topic, that's fine.

I don't buy that. The direct quote is "For the Rust projects, I suggest trying to start with cargo-pgo.". This phrasing specifically mentions the SVSM in "the Rust projects". To me this doesn't read at all as "use cargo-pgo to play around with PGO on some toy projects" and it reads much more as "for the SVSM, I suggest trying to start with cargo-pgo".

Feel free to not believe me :) I just shared my thoughts behind this phrase. However, I appreciate this feedback - I need to tune my template to explicitly mention Rustc docs about PGO for cases where cargo-pgo is not enough.

There are some gains to be had by LTO, but my estimate would be that we'd gain less than the average Rust project. PGO is completely inappropriate given the state of the project. If we wanted to increase performance, there'd be a ton of other low-hanging fruit we should rather pursue than doing a bunch of work to get PGO to work.

If your estimate is enough to ignore enabling LTO for SVSM - it's completely fine! Project devs are free to use whatever estimation mechanism they want. The same applies to PGO: if you think that PGO brings less value than requires efforts to integrate - just close (or better postpone/leave open) the issue with such a comment. Different projects have different priorities, human resources, and zillion other factors - I respect such choices with no doubt.

But that's my point. I feel like you don't look much at the project and just send out issues without much thought. We don't excuse Nigerian prince email scams by saying that people can just delete them if they aren't interested. Spam is spam. If you send out a lot of issues without much though, you're bound to cause some projects harm.

Projects are completely free to report me if I harm their workflows in one or another way. I haven't seen such situations before though (maybe SVSM is the first one). If project maintainers decide that LTO/PGO are harmful for their software - they are free to comment it in any way and just ignore my suggestions.

Again, you don't seem to value the quality of the issues you open and seem entirely focused on quantity.

I am sure that my issues have a good enough quality in average. If you disagree - you can tell me about how the issue format can be improved. I will try to analyze your feedback, consider other factors (like the amount of my efforts) and maybe change the format. I am always open to the technical-driven feedback

Regarding quantity. Since my aim is improving the whole ecosystem (and we are yet ready to enable LTO in Cargo), I will try to integrate LTO into as much as possible projects. Yep, it means enabling it manually in hundreds of projects - I don't have problems with it.

Also, your response is shifting the goalpost. You claimed that the SVSM was using many dependencies, I claimed that it wasn't. Whether or not 29 is enough to justify LTO was never in question, the question was about whether 29 dependencies is a lot. Given that you work with a lot of Rust-based projects, you should know that 29 is not considered to be a lot by any standard.

No shift here - 29 dependencies is a lot of dependencies from my point of view. I don't have problems with many deps for projects - I just want to tell you that LTO helps with performing cross-crate deps optimizations. I don't care about comparing the average amount of deps between Rust projects - ofc I've seen Rust projects with hundreds of deps. For me it doesn't matter - 29 deps or 229 deps you have - LTO is still the thing in both cases.

I don't believe you. Name the 29 dependencies. Until you correctly name the 29 dependencies, I don't believe another word out of your mouth.

You are free to believe or not in whatever you want, of course :)

You're not a part of the community for the vast, vast majority of the projects you contribute these kinds of issues to. You have never before interacted with most of them, you don't use at least some of them (I highly doubt you were ever using this project or were planing on doing so in the future). If you're interested in becoming part of a community, that's great, but opening an issue and then immediately leaving, isn't the way to do it.

I disagree with this point of view. Even if I don't use these projects right now, there is a chance that I will use it eventually (or my system will use it indirectly) or my friends will use some of them or my work environment (present and future) will use it. This is why I want to have them to be more optimized by default - that's a benefit not only for me but for all their users.

Regarding being a part of the community - it's up to discussion and definitions. I don't care much about formal recognition as a community member - my own understanding of this question is enough to contribute my time into it in this way.

Allow me to clarify: Please consider contributing to Rust and cargo directly and not just primarily to ecosystems using it. Any impact you make in Rust and cargo will be much greater and will take away fewer resources from other projects.

I understand your point of view. My final aim is to enable LTO by default in Cargo. Before that point (since Cargo devs are a bit conservative right now) I will continue improving the ecosystem, by integrating LTO to projects one by one. After collecting enough examples of "enabled LTO for projects" I will show it to the Cargo dev team as an argument to consider this decision once again. Right now they are not ready for this decision - and I will improve the ecosystem "manually".

You have no chance in this thread to change my mind about my strategy for the creation LTO/PGO issues - that's my way and I know what I want to achieve with such a move. I will continue creating such requests (and PRs from time to time). However, if you have other suggestions about how to increase the LTO/PGO enablement in the industry (except direct contribution into Cargo/Rustc - I got your point, and I agree with it, it's just not a right to do it yet) - I would be happy to discuss them with you.

Freax13 commented 1 week ago

I don't believe you. Name the 29 dependencies. Until you correctly name the 29 dependencies, I don't believe another word out of your mouth.

You are free to believe or not in whatever you want, of course :)

I rest my case, you're just lying.

You have no chance in this thread to change my mind about my strategy for the creation LTO/PGO issues - that's my way and I know what I want to achieve with such a move.

Admitting that you're not even considering that you might be in the wrong isn't necessarily a good thing...

whitequark commented 5 days ago

Admitting that you're not even considering that you might be in the wrong isn't necessarily a good thing...

(I suggest reporting him until GitHub takes action.)

Freax13 commented 5 days ago

(I suggest reporting him until GitHub takes action.)

I reported him last week. I haven't heard anything back from GitHub though they also said that it could take a while for them to respond :-/