Make builder tracing work and switchable at runtime - #2802
Conversation
| Bundle-Name: %pluginName | ||
| Bundle-SymbolicName: org.eclipse.core.resources; singleton:=true | ||
| Bundle-Version: 3.24.100.qualifier | ||
| Bundle-Version: 3.25.100.qualifier |
There was a problem hiding this comment.
New API, hence the bump.
There was a problem hiding this comment.
The bump shouldn't include service segment!
There was a problem hiding this comment.
The point is that a bump of the minor version sets the service version back to zero so it should be 3.25.0 not 3.25.100
There was a problem hiding this comment.
I think in the past we had the rule to use 100 in the end to allow downports. Is that obsolete? https://github.com/eclipse-platform/eclipse.platform/blob/master/docs/VersionNumbering.md#when-to-change-the-service-segment
There was a problem hiding this comment.
Forget this comment, this is for the first change of the third segment. Not the other one, so 3.25.0 would be correct.
I leave this as it is for now as we anyhow want tracing and not new API.
dd12d64 to
05a9001
Compare
|
Have you considered providing tracing information instead? For the reasons mentioned in #2801 it should be sufficient. There is also "Event view" in runtime thac can be extended for that, or just create something similar based on that. My main concern with the proposal here is that except for occasional performance checks it is not needed (it wasn't obviously requested by anyone since 20 years of resources API), however it will always generate lot more events (per builder & per project). |
|
Thanks @iloveeclipse for the suggestion I look into tracing as an alternative. This might take a bit longer, as I soon will vanish into summer vacation and because I use this in a similar version already since a while in my custom aggregator build so my personal pressure is not that high to rework that approach. |
vogella
changed the title
05a9001 to
582ebd6
Compare
vogella
changed the title
582ebd6 to
f12accc
Compare
vogella
changed the title
f12accc to
3f478b7
Compare
|
Implemented the suggestions from @merks and @iloveeclipse Please check again. |
Please wait till next week, I'm getting old and need some rest on weekends :-) |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect and incomplete performance tracing for workspace builders under parallel execution, and enables toggling tracing at runtime via DebugOptions (instead of being frozen at startup). It updates org.eclipse.core.runtime to keep PerformanceStats.ENABLED in sync with debug options, and updates org.eclipse.core.resources to use per-run handles so start/end timing is thread-safe and correctly attributed to builder + project context.
Changes:
- Make
PerformanceStats.ENABLEDruntime-switchable and rework threshold/success-tracing evaluation to reflect current debug options. - Replace
ResourceStats’ single shared “current” stats with per-runRunhandles to avoid cross-thread overwrites and context/hash corruption. - Add
BuilderTracingTestcoverage and wire it into the resources builder test suite.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/runtime/PerformanceStats.java | Makes tracing enablement dynamic; adjusts threshold handling and listener registration behavior. |
| runtime/bundles/org.eclipse.core.runtime/src/org/eclipse/core/internal/runtime/InternalPlatform.java | Registers a DebugOptionsListener to refresh runtime debug flags and keep PerformanceStats.ENABLED synchronized. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/ResourceStats.java | Introduces per-run Run handle model; adds dynamic option refresh and per-event tracing predicates. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/BuildManager.java | Uses Run handles for builder timing; adds threshold-based logging behavior for builder debug/trace. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/NotificationManager.java | Switches listener timing to Run handles and dynamic isTracingListeners() checks. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/SaveManager.java | Wraps save participant and snapshot timing in Run handles for correct pairing. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java | Migrates refresh timing to Run handles and uses returned duration for conditional logging. |
| resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/Policy.java | Triggers ResourceStats.optionsChanged() on debug option changes so resources tracing can be toggled at runtime. |
| resources/bundles/org.eclipse.core.resources/.options | Documents new builder-threshold semantics (including “0 gathers stats without logging”). |
| resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/BuilderTracingTest.java | New test validating runtime toggling, attribution, and listener notification. |
| resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/AllBuilderTests.java | Adds BuilderTracingTest to the builder test suite. |
| resources/tests/org.eclipse.core.tests.resources/META-INF/MANIFEST.MF | Adds debug options package import needed by the new test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
86652c1 to
f2f88e7
Compare
Its been now a month. :-) Any new feedback? It is based on tracing as requested by you and @merks The tracing can be turned on and off at runtime. No additional event. |
Please provide steps to validate the PR. Which options need to be set? |
|
At least for me I see builders info appearing in only after setting these tracing options: Please confirm that this is the right way to validate. |
| legacyPreferencesService = context.registerService(ILegacyPreferences.class, new InitLegacyPreferences(), new Hashtable<>()); | ||
|
|
||
| Hashtable<String, String> debugProperties = new Hashtable<>(2); | ||
| debugProperties.put(DebugOptions.LISTENER_SYMBOLICNAME, Platform.PI_RUNTIME); |
There was a problem hiding this comment.
I miss contribution to the extension point so that users can enable runtime performance tracing at runtime, something like this:
<extension
point="org.eclipse.ui.trace.traceComponents">
<component
id="org.eclipse.core.runtime.traceComponent" label="%traceComponentName">
<bundle name="org.eclipse.core.runtime" />
</component>
</extension>
There was a problem hiding this comment.
Components can disable/ activate it via:
private void disableTracing() {
ServiceCaller.callOnce(getClass(), DebugOptions.class, options -> {
if (!debugWasEnabled) {
// disabling debug discards all options, including the ones set above
options.setDebugEnabled(false);
return;
}
replacedOptions.forEach((option, value) -> {
if (value == null) {
options.removeOption(option);
} else {
options.setOption(option, value);
}
});
});
}
Enable see here https://github.com/eclipse-platform/eclipse.platform/pull/2896/changes#diff-a6abe37830234d56ff13e2c61b8e7f026fd3ddc40adaa29bbba2e44e79471c2eR111
No need for plugin.xml.
The above coding is copied from my example view #2896
There was a problem hiding this comment.
Components can disable/ activate it via:
I'm talking about enabling tracing by users, not code. It should be possible to activate it by users, like with other bundles which provide tracing?
|
In general looks good, assuming I've used it as expected, see my comments above. |
There was a problem hiding this comment.
🟡 Changes recommended
Parallel statistics updates remain unsafe, and repeated failures can lose listener notifications.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
f2f88e7 to
53bcdae
Compare
|
@iloveeclipse let me know if you have more questions, I think I answered all. |
53bcdae to
c6c9938
Compare
|
I miss tracing activation possibility by users, see above. |
There was a problem hiding this comment.
🟡 Changes recommended
Null-context events are double-counted, and debug-only builder timing can incorrectly produce performance logs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
|
These are existing trace options, which are part of the launch configuration. 
Once this is merged, I plan to promote my build monitor from draft to real PR to make it easier for the user to see the build time of the individual builders. Another way for the user to activate this is not planned by me. |
Per-builder tracing already existed as org.eclipse.core.resources/perf/builders, blaming the builder and using the project as context, but it never worked. ResourceStats kept the in-progress PerformanceStats in a single static field, so under parallel builds two builders overwrote each other's start time and any notification triggered from inside a builder dropped that builder's timing completely. PerformanceStats.startRun(context) also mutates the context of the shared stats instance, and since context is part of its hashCode that corrupted the key of an object already in the global stats map, so per-builder totals never aggregated. Each start now returns a handle that the matching end consumes, so runs are timed independently across threads and nesting levels, and the elapsed time is recorded through PerformanceStats.addRun, which leaves the shared context alone. The racy BuildManager.timeStamp field used for build/invoking output is gone as well. A builder run reaching the perf/builders threshold is logged like the project refresh tracing added earlier, including the build kind, and the refresh case reports the duration of the single run rather than the cumulative total. Tracing enablement now follows the debug options instead of being frozen at startup, so tooling can switch it on without a restart: core.runtime registers a debug options listener that updates PerformanceStats.ENABLED, and the resources trace flags are refreshed from the listener core.resources already had. Because a bundle is only notified about its own options, the resources flags deliberately track only perf/* of core.resources and consult the global flag separately, which also makes a disable followed by a re-enable work. The threshold is now the shortest duration still reported, so a threshold of 0 reports every occurrence to performance listeners while writing nothing to the performance log, which lets tooling gather statistics without flooding the log. The processor keeps one pending entry per occurrence instead of one per event, so repeated runs within its delay are no longer collapsed into a single notification, and the counters are updated under a lock since runs of the same event can now end concurrently. Listener statistics are removed regardless of the tracing state, so a listener removed while tracing is off leaves no stale entry behind. Assisted-by: multiple AI agents and layers of automated tooling 🤖
c6c9938 to
d6ae001
Compare
|
User currently has no way to change runtime bundle, trwcing options, see my comment above which. Please simply add the contribution to plugin xml, it is fully unrelated to additional bundle or view or whatever. |
|
Why should this tracing option be special in how the user can activate it? We have tons of tracing options. |
Exact. It shouldn't be special, but it is special today because it is not exposed to user. You can only set it from debugger, but not from IDE, unlike many other tracing options. Just try to find it in your IDE. |
Replaces the earlier
PRE_PROJECT_BUILD/POST_PROJECT_BUILDproposal with the tracing approach suggested by @iloveeclipse and @merks, so no new API is needed.Per-builder tracing already existed as
org.eclipse.core.resources/perf/builders, blaming the builder and using the project as context, but it never worked:ResourceStatskept the in-progressPerformanceStatsin a single static field, so under parallel builds two builders overwrote each other's start time and any notification triggered from inside a builder dropped that builder's timing completely.PerformanceStats.startRun(context)also mutates the context of the shared stats instance, and since context is part of itshashCodethat corrupted the key of an object already in the global stats map, so per-builder totals never aggregated. Each start now returns a handle that the matching end consumes, and the elapsed time is recorded throughaddRun, which leaves the shared context alone.The second half is that tracing enablement was frozen at startup, which made it useless for tooling.
PerformanceStats.ENABLEDis now kept in sync with the debug options by a listener in core.runtime, and the resources trace flags are refreshed from the listener core.resources already had, so tracing can be switched on without a restart. Note that a bundle is only notified about its own options, so the resources flags track only its ownperf/*entries and consult the global flag separately; that is also what makes disabling and re-enabling work. The threshold is now the shortest duration still reported, so 0 means "every occurrence", and for builders a threshold of 0 gathers statistics without writing anything to the log.With this in place the existing "Event Spy" view in
org.eclipse.core.toolsalready shows correct per-builder and per-project build times, since it readsPerformanceStats.getAllStats(). A dedicated Build Monitor view is planned fororg.eclipse.ui.monitoringin eclipse.platform.ui, which turns builder tracing on while the view is open and restores the previous debug options when it closes; that needs this change first, so it will be a follow-up PR there.BuilderTracingTestcovers the runtime toggle, the per-builder and per-project attribution, and the listener notification, none of which was testable before enablement became dynamic.Removing
finalfromPerformanceStats.ENABLEDreports 0 API errors with-Papi-check, but it does change the documented promise that enablement cannot change during the life of the platform, so that part deserves a look.