Tracing work #40634
Tracing work #40634
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
elibarzilay
force-pushed
the
tracing-work
branch
2 times, most recently
from
c336cd7 to
32d9923
Compare
src/compiler/tracing.ts
Outdated
| performance.mark("beginTracing"); | ||
| fs.writeSync(traceFd, `{"pid":1,"tid":1,"ph":"i","cat":"${phase}","ts":${1000 * timestamp()},"name":"${name}","s":"g","args":{ "ts": ${JSON.stringify(args)} }},\n`); | ||
| fs.writeSync(traceFd, `,{"pid":1,"tid":1,"ph":"${eventType}","cat":"${phase}","ts":${time},"name":"${name}"`); |
There was a problem hiding this comment.
Why did this comma need to move? If it has to be this way, maybe we could move the \n too so that the comma is still at EOL?
There was a problem hiding this comment.
The resulting dump will look like:
,{...}
,{...}
,{...}
,{...}
]
so this makes it possible to have decent looking output and not worry about aborting or special casing the last value. I normally dislike this style too, but I want to avoid special cases to avoid as much runtime overhead as possible, and avoid complicating the code for something that is rarely used (looking at the dump text directly).
There was a problem hiding this comment.
Right, but what if it was ,\n{"pid"? Then the comma still wouldn't be inserted without a following item, but it would appear on the right line.
elibarzilay
force-pushed
the
tracing-work
branch
2 times, most recently
from
03a2606 to
eb3f357
Compare
src/compiler/tracing.ts
Outdated
| performance.mark("beginTracing"); | ||
| fs.writeSync(traceFd, `{"pid":1,"tid":1,"ph":"i","cat":"${phase}","ts":${1000 * timestamp()},"name":"${name}","s":"g","args":{ "ts": ${JSON.stringify(args)} }},\n`); | ||
| fs.writeSync(traceFd, `,{"pid":1,"tid":1,"ph":"${eventType}","cat":"${phase}","ts":${time},"name":"${name}"`); |
There was a problem hiding this comment.
Right, but what if it was ,\n{"pid"? Then the comma still wouldn't be inserted without a following item, but it would appear on the right line.
elibarzilay
force-pushed
the
tracing-work
branch
from
eb3f357 to
5dbf176
Compare
|
@amcasey: I don't know why gh won't let me reply to the line comment -- the thing that I dislike is to have the implicit invariant that we're at the EOL after output, since that leads more easily to a mess later on. But anyway, that's not nearly a strong enough of an opinion, so I just changed it... |
* Fix: `E` events need to have the same information that is on the
corresponding `B` events. (Superseded below.)
* Use `I` (not `i`) for instant events, so they show in devtools
too. (Though they don't go through the flame chart as they do in
`about://tracing`, so they're not nearly as useful.)
* Abstract the code that writes the records in a single `writeEvent`
local function.
* Make `args` optional, and default to `undefined` (which will not add
them) at all.
* Drop the `{ "ts": ... }` wrapper around the `args`, after verifying
that having arguments with names like `begin`, `end`, `pos`, `id`
doesn't interfere with either UIs.
* Add `tracing.push`/`tracing.pop` for complete events, change a few
`.begin`/`.end` to use these. (The caveat is that until there's an exit
handler to dump unterminated events, these won't show in the dump. When
that's done the push/pop variant can be used everywhere.)
* Add meta lines to name the process and the thread, and a line that
avoids the warning when opening in devtools.
elibarzilay
force-pushed
the
tracing-work
branch
from
5dbf176 to
f1f4a3c
Compare
Eevents need to have the same information that is on thecorresponding
Bevents.Use
I(noti) for instant events, so they show in devtoolstoo. (Though they don't go through the flame chart as they do in
about://tracing, so they're not nearly as useful.)Abstract the code that writes the records in a single
spitEventlocal function.
Make
argsoptional, and default toundefined(which will not addthem) at all.
Drop the
{ "ts": ... }wrapper around theargs, after verifyingthat having arguments with names like
begin,end,pos,iddoesn't interfere with either UIs.
Add
tracing.push/tracing.popfor complete events, change a few.begin/.endto use these. (The caveat is that until there's an exithandler to dump unterminated events, these won't show in the dump. When
that's done the push/pop variant can be used everywhere.)
Add meta lines to name the process and the thread, and a line that
avoids the warning when opening in devtools.