-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix #7459 -- allow parallel timers of the same name. #9392
Conversation
var timers; // timer handles | ||
timers = PerfUtils.markStart([timerBuildFull, timerBuildPart]); | ||
timerBuildFull = timers[0]; | ||
timerBuildPart = timers[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too bad Chromium does not support destructuring assignments.
Yeah, I'm so glad that somebody picked this up. 👍 |
@busykai Is this to get rid of the "Performance measurement {id} is already defined" error message? If so, do you have a specific scenario that causes this problem? Every time I have seen this problem it's because the calls to start ( Your code will eliminate the error message in that case, but the timer will still never end, so the results will be skewed. And now we won't get an error message to indicate that something's broken. FYI, you can have parallel timers with current code, but it's up to caller to generate unique names. |
@redmunds, this is to get rid of "Recursive tests with the same name are not supported...", a different situation. A simple use case is when you have a "slow" linter installed and quickly switch back and forth between files. In some cases (e.g. XDK where the fs is slower or when we were opening binary files in Brackets) you could see the same thing from the fs (I guess it was Here's a simple patch which would always inflict the problem described in #7459:
Yes, but doing that type of book keeping in all the places is a lot of extra code. Most of the callers would simply generate a descriptive string and pass it to the |
if (id) { | ||
this.id = id; | ||
} else { | ||
this.id = (reent) ? "[reent " + this.reent + "] " + name : name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.reent
is used before being set.
I wrote this extension to test this: /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, brackets */
define(function (require, exports, module) {
"use strict";
var CommandManager = brackets.getModule("command/CommandManager"),
Menus = brackets.getModule("command/Menus"),
PerfUtils = brackets.getModule("utils/PerfUtils");
var timerId = "redmunds.timer";
function test() {
var perfTimer1 = PerfUtils.markStart(timerId),
perfTimer2 = PerfUtils.markStart(timerId);
window.setTimeout(function () {
PerfUtils.addMeasurement(perfTimer1);
PerfUtils.addMeasurement(perfTimer2);
var perfData = PerfUtils.getData();
window.console.log("perfTimer1: " + perfData[perfTimer1]);
window.console.log("perfTimer2: " + perfData[perfTimer2]);
window.console.log(timerId + ": " + perfData[timerId]);
}, 100);
}
Menus.getMenu("debug-menu").addMenuItem(
CommandManager.register("Recursive PerfUtils Test", "redmunds.test", test)
);
}); When I run it, the results written to console are:
Why is I expected |
I played around with this some more, and I guess |
@redmunds, thanks for bringing up this use case. The reason for the second timer being undefined is because I use the fact that the array key is always a /*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, brackets */
define(function (require, exports, module) {
"use strict";
var CommandManager = brackets.getModule("command/CommandManager"),
Menus = brackets.getModule("command/Menus"),
PerfUtils = brackets.getModule("utils/PerfUtils");
var timerId = "redmunds.timer";
function test() {
var perfTimer1 = PerfUtils.markStart(timerId),
perfTimer2 = PerfUtils.markStart(timerId);
window.setTimeout(function () {
PerfUtils.addMeasurement(perfTimer1);
PerfUtils.addMeasurement(perfTimer2);
var perfData1 = PerfUtils.getData(perfTimer1);
var perfData2 = PerfUtils.getData(perfTimer2);
var perfData = PerfUtils.getData();
window.console.log("perfTimer1: " + perfData1);
window.console.log("perfTimer2: " + perfData2);
// NB: here is the issue, the "opaque" timer object should have the same
// behaviour as if timerId was used.
window.console.log(timerId + ": " + perfData[timerId]);
}, 100);
}
Menus.getMenu("debug-menu").addMenuItem(
CommandManager.register("Recursive PerfUtils Test", "redmunds.test", test)
);
}); So I guess I will use explicit |
Yes, that makes sense |
This is to preserve the API compatibility. Also: o make the aux function name more descriptive.
@redmunds, addressed your comments. |
Thanks! Merging. |
Fix #7459 -- allow parallel timers of the same name.
Yea! :D |
Fix #7459.
Keep track of the measurements of the same name started in parallel: assign them different tracking ids, but report them under the same name. All the measurements are now
PerfMeasurement
objects. Also, code cleanup.CC: @redmunds, @peterflynn -- please take a look if this change is a good idea.