[S] Change in dart/sdk[main]: Optimize LibraryContext.remove.
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
if (removed.isEmpty) return;This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
Related details
- Morgan :)
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) added 1 comment![]( "Open in Gerrit")
if (removed.isEmpty) return;This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
I added the special case for the null cycle to make the unit test pass; it's possible that the correct fix for the unit test failures was to update the unit test state.
On large codebases (5000 files) it looks like the analyzer (via build_runner) is spending almost as much time calling "remove" as doing analysis. So we can definitely benefit from some optimization here :) I posted a few screenshots on the linked bug, obviously I can share detail+repro.
Thanks.
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
if (removed.isEmpty) return;Morgan :)This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
I added the special case for the null cycle to make the unit test pass; it's possible that the correct fix for the unit test failures was to update the unit test state.
On large codebases (5000 files) it looks like the analyzer (via build_runner) is spending almost as much time calling "remove" as doing analysis. So we can definitely benefit from some optimization here :) I posted a few screenshots on the linked bug, obviously I can share detail+repro.
Thanks.
There are two main changes: `if removed.isEmpty` and the rest.
Do we get not enough optimized by `isEmpty` check?
Related details
- Morgan :)
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) added 1 comment![]( "Open in Gerrit")
if (removed.isEmpty) return;Morgan :)This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
Konstantin ShcheglovI added the special case for the null cycle to make the unit test pass; it's possible that the correct fix for the unit test failures was to update the unit test state.
On large codebases (5000 files) it looks like the analyzer (via build_runner) is spending almost as much time calling "remove" as doing analysis. So we can definitely benefit from some optimization here :) I posted a few screenshots on the linked bug, obviously I can share detail+repro.
Thanks.
There are two main changes: `if removed.isEmpty` and the rest.
Do we get not enough optimized by `isEmpty` check?
No, they help with different cases:
- isEmpty is not relevant to build_runner, it helps if you have many drivers
- the more complex optimization helps any file change based on codebase size, this is the one that speeds up build_runner
I can split them up if you like :)
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
if (removed.isEmpty) return;Morgan :)This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
Konstantin ShcheglovI added the special case for the null cycle to make the unit test pass; it's possible that the correct fix for the unit test failures was to update the unit test state.
On large codebases (5000 files) it looks like the analyzer (via build_runner) is spending almost as much time calling "remove" as doing analysis. So we can definitely benefit from some optimization here :) I posted a few screenshots on the linked bug, obviously I can share detail+repro.
Thanks.
Morgan :)There are two main changes: `if removed.isEmpty` and the rest.
Do we get not enough optimized by `isEmpty` check?
No, they help with different cases:
- isEmpty is not relevant to build_runner, it helps if you have many drivers
- the more complex optimization helps any file change based on codebase size, this is the one that speeds up build_runnerI can split them up if you like :)
Yes, I think splitting and having separate issue for discussing the second portion would be useful.
Related details
- Morgan :)
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) voted and added 1 comment![]( "Open in Gerrit")
Votes added by Morgan :)
| Auto-Submit | +1 |
1 comment
if (removed.isEmpty) return;Morgan :)This part was discussed and makes sense to me.
The rest, I'm not sure.
I don't like using `internal_libraryCycle`, this breaks layering.
I don't like special-casing was / was-not cycle created.
If there is no cycle, we could not have added it to loaded bundles.
Konstantin ShcheglovI added the special case for the null cycle to make the unit test pass; it's possible that the correct fix for the unit test failures was to update the unit test state.
On large codebases (5000 files) it looks like the analyzer (via build_runner) is spending almost as much time calling "remove" as doing analysis. So we can definitely benefit from some optimization here :) I posted a few screenshots on the linked bug, obviously I can share detail+repro.
Thanks.
Morgan :)There are two main changes: `if removed.isEmpty` and the rest.
Do we get not enough optimized by `isEmpty` check?
Konstantin ShcheglovNo, they help with different cases:
- isEmpty is not relevant to build_runner, it helps if you have many drivers
- the more complex optimization helps any file change based on codebase size, this is the one that speeds up build_runnerI can split them up if you like :)
Yes, I think splitting and having separate issue for discussing the second portion would be useful.
I sent https://dart-review.googlesource.com/c/sdk/+/487160 for the early return and removed it here.
Then I reverted this change to just the optimization, without the special case. You will see it causes some CQ failures, would it be correct to update the test to fix those?
Or we can discuss further on the issue. It currently covers three possible optimizations: deduplicating events (only relevant when there are many events!), the early return (many drivers) and this one (many files). I can add detail and/or split out issues as you like, please let me know. Thanks :)
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
I don't know out of head why this happens.
It looks that one of the sanity check triggerred.
I think we need to find the root cause.
Related details
- Morgan :)
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) added 1 comment![]( "Open in Gerrit")
Happy to dig into it.
First though I wanted to take a step back and discuss why it matters, so hopefully we can agree that some fix is needed :)
I added a lot more detail on
https://github.com/dart-lang/sdk/issues/62760#issuecomment-4045624349
please take a look and see what you think.
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) added 1 comment![]( "Open in Gerrit")
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morgan :) (Gerrit)
Morgan :) abandoned this change.
Morgan :) abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |