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. |
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. |
450c519 to
21287df
Compare
|
Added, thanks. org.eclipse.core.runtime now contributes an org.eclipse.ui.trace.traceComponents component, so its perf and perf/success options can be set from Preferences > General > Tracing instead of only from a launch configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
Positive threshold semantics regress at the equality boundary, and key aggregate and repeated-notification behavior lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/builders/BuilderTracingTest.java:151
- This predicate verifies only the contextual per-project row, so it does not cover the claimed per-builder aggregate attribution. A regression that stops recording the context-free builder total would still pass; assert both a null-context aggregate and the project-context row.
assertTrue(Arrays.stream(PerformanceStats.getAllStats())
.anyMatch(stats -> EVENT_BUILDERS.equals(stats.getEvent())
&& stats.getBlameString().contains(SortBuilder.class.getSimpleName())
&& project.getName().equals(stats.getContext())),
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Balanced
Per-builder tracing existed as org.eclipse.core.resources/perf/builders but never worked. ResourceStats kept the in-progress PerformanceStats in a single static field, so parallel builders overwrote each other's start time, and startRun(context) mutated the context of that shared instance, which is part of its hashCode and so corrupted its key in the global stats map. Each start now returns a handle that the matching end consumes, and the duration is recorded through addRun, which leaves the shared context alone. Enablement is no longer frozen at startup: core.runtime updates PerformanceStats.ENABLED from a debug options listener, and the resources flags are refreshed from the listener core.resources already had. Because a bundle is only notified about its own options, the resources flags track just perf/* of core.resources and read the global flag separately. The threshold is now the shortest duration still reported, so 0 reports every occurrence to listeners without writing to the performance log. core.runtime also contributes an org.eclipse.ui.trace.traceComponents component, like core.jobs and core.resources do, so its perf options can be set from the Tracing preference page instead of only from a launch configuration. Assisted-by: multiple AI agents and layers of automated tooling 🤖
21287df to
74c6650
Compare
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, andstartRun(context)mutated the context of the shared instance, which is part of itshashCode, corrupting the key of an object already in the global stats map. Each start now returns a handle that the matching end consumes, and the elapsed time is recorded throughaddRun, which leaves the shared context alone.Tracing enablement was also frozen at startup, which made it useless for tooling.
PerformanceStats.ENABLEDnow follows the debug options and the resources trace flags are refreshed from the listener core.resources already had, so tracing can be switched on and off without a restart.org.eclipse.core.runtimenow contributes a trace component like core.jobs and core.resources already do, so itsperfoptions are reachable from the Tracing preference page instead of only from a launch configuration.To validate: enable tracing for "Platform Core Runtime" (
perf,perf/success) and "Platform Core Resources" (perf/builders), then trigger a build. Aperf/buildersthreshold of 0 gathers statistics for every run without writing anything to the log, and the existing Event Spy view inorg.eclipse.core.toolsshows the resulting per-builder and per-project times.BuilderTracingTestcovers the runtime toggle, the per-builder and per-project attribution, and the listener notification, none of which was testable before enablement became dynamic.