Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate the use of BigInt or as string in the TSP #41

Closed
tahini opened this issue May 25, 2020 · 3 comments
Closed

Investigate the use of BigInt or as string in the TSP #41

tahini opened this issue May 25, 2020 · 3 comments
Assignees
Labels
help wanted Extra attention is needed Theia UI frontend trace server protocol Needs changes to the trace-server-protocol itself Trace Server Involve changes in the trace server itself (in the incubator source code)
Milestone

Comments

@tahini
Copy link
Contributor

tahini commented May 25, 2020

  • Sending "long" times using JSON number is a potential problem to any javascript or any language that doesn't support big integer. It will work in Theia if we use BigInt, but not in other frontend if the dev doesn't or can't support BigInt.
  • Sending as string might impact the size or the response but need to test if it really does something. The advantage is that the frontend can convert that string in something that can be useful
  • We can either force BigInt in TSP or use String
  • NodeJS 10 and Chrome supports it, (JavaScript version?)
  • Time Graph library uses currently offset scheme
@tahini tahini added help wanted Extra attention is needed Trace Server Involve changes in the trace server itself (in the incubator source code) trace server protocol Needs changes to the trace-server-protocol itself Theia UI frontend labels May 25, 2020
@tahini tahini added this to the 1.0 milestone May 25, 2020
@haoadoreorange
Copy link

I forgot that there is an issue for this. A bit update to the current state.

The JSON format itself supports BigInt. However, the JSON.parse spec of JS can't parse those value (it loses precision). I retraced some old TC39 proposal but it seems dead. I decided going for a custom parser.

By searching around I find the package json-bigint the most suitable, the package itself was inspired by the original JSON now part of V8. I made a PR with some changes and optimization. Benmarking results a ~4x slower than the default parser, which is pretty optimized among custom parsers afaik. The PR is now merged and published on npm.

Where do we go from here ?

@MatthewKhouzam
Copy link
Contributor

Maybe we can look into this: https://www.npmjs.com/package/timestamp-nano

PatrickTasse added a commit to PatrickTasse/theia-trace-extension that referenced this issue Sep 16, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart navigation is set to 2 ns, to be able
to zoom out of it and to always have at least one time axis tick fully
visible.

Fixes Issue eclipse-cdt-cloud#41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
PatrickTasse added a commit to PatrickTasse/theia-trace-extension that referenced this issue Sep 22, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart and toolbar button zooming is set to
2 ns, to be able to zoom out of it and to always have at least one time
axis tick fully visible.

Fixes Issue eclipse-cdt-cloud#41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
PatrickTasse added a commit to PatrickTasse/theia-trace-extension that referenced this issue Oct 4, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart and toolbar button zooming is set to
2 ns, to be able to zoom out of it and to always have at least one time
axis tick fully visible.

Fixes Issue eclipse-cdt-cloud#41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
PatrickTasse added a commit to PatrickTasse/theia-trace-extension that referenced this issue Oct 5, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart and toolbar button zooming is set to
2 ns, to be able to zoom out of it and to always have at least one time
axis tick fully visible.

Fixes Issue eclipse-cdt-cloud#41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
PatrickTasse added a commit that referenced this issue Oct 6, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart and toolbar button zooming is set to
2 ns, to be able to zoom out of it and to always have at least one time
axis tick fully visible.

Fixes Issue #41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
@ebugden
Copy link
Contributor

ebugden commented Oct 13, 2021

Closing since resolved by PR #485.

Utility of this fix: My understanding was not having BigInt precision on timestamps was making selection inaccurate. MatthewKhouzam: "we could not seek accurately without that patch. you would have 512 ns errors on your clicks"

@ebugden ebugden closed this as completed Oct 13, 2021
hriday-panchasara pushed a commit to hriday-panchasara/theia-trace-extension that referenced this issue Nov 10, 2021
Use type 'bigint' for all variables that are timestamps or durations.

Rename TimeRange.getstart() to getStart().

The minimum view range in XY chart and toolbar button zooming is set to
2 ns, to be able to zoom out of it and to always have at least one time
axis tick fully visible.

Fixes Issue eclipse-cdt-cloud#41

Signed-off-by: Patrick Tasse <patrick.tasse@ericsson.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed Theia UI frontend trace server protocol Needs changes to the trace-server-protocol itself Trace Server Involve changes in the trace server itself (in the incubator source code)
Projects
None yet
Development

No branches or pull requests

5 participants