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

Comments on "Define 'relative high resolution time'" #96

Closed
annevk opened this issue Sep 21, 2020 · 13 comments · Fixed by #97
Closed

Comments on "Define 'relative high resolution time'" #96

annevk opened this issue Sep 21, 2020 · 13 comments · Fixed by #97

Comments

@annevk
Copy link
Member

annevk commented Sep 21, 2020

Looking at the diff of #95 there's a couple things I don't really understand:

  1. "relative high resolution time" says it takes a DOMHighResTimeStamp but this isn't used and a variable isn't named for it. Should that be removed?
  2. "current high resolution time" talks about a "context object" but that is only available (as "this" these days) when defining IDL members.
@yoavweiss
Copy link
Contributor

  1. "relative high resolution time" says it takes a DOMHighResTimeStamp but this isn't used and a variable isn't named for it. Should that be removed?

Seems like we phrased it in a way that's not clear. Should've followed https://infra.spec.whatwg.org/#declaration

2. "current high resolution time" talks about a "context object" but that is only available (as "this" these days) when defining IDL members.

"current high resolution time" is called from now(), so I assumed we can use "context object" there. What's a better way to define this?

/cc @igneel64

@annevk
Copy link
Member Author

annevk commented Sep 21, 2020

@yoavweiss you would pass "this" as an argument in from now() and say that "current high resolution time" takes an object of the type that now() is on. That might not make it as reusable as you want though so maybe it should take a global as well and you pass this's relevant global object in?

@npm1
Copy link
Contributor

npm1 commented Sep 21, 2020

One thing that's not clear to me is how where time is the present time gets transformed into a DOMHighResTimestamp. Isn't this where some form of rounding/jittering must be performed?

@annevk
Copy link
Member Author

annevk commented Sep 21, 2020

That's a good point, though I think ideally we add noise directly at the source of the time signal, to allow exposing it through other types in the future.

@yoavweiss
Copy link
Contributor

Note to self: "current high resolution time" is also called from https://wicg.github.io/element-timing/#ref-for-report-image-element-timing%E2%91%A0, so it makes sense to pass along a global.

@npm1 - what you're suggesting could be an evolution path towards #93 (where we add explicit rounding/jitter, and then change it for isolated contexts)

@npm1
Copy link
Contributor

npm1 commented Sep 21, 2020

It's used in more places than that. Other examples:

@igneel64
Copy link
Contributor

igneel64 commented Sep 22, 2020

@annevk
Thanks for spotting the definition of relative high resolution time. Will be fixed based on the spec, I missed that sorry.

As @npm1 and @yoavweiss mentioned, we would want the definition of current high resolution time to make sense for all the places that it is mentioned/reference, even if we need to make some small reference fixes on the other specs directly.

Do you think that a definition like the one below would be enough ? (small change on this)

The current high resolution time returns the result of relative
high resolution time where time is the present time 
and global is a global object.

Does this case conform if we define what the global is in other places like element timing , resource timing etc ?

igneel64 added a commit to igneel64/hr-time that referenced this issue Sep 22, 2020
Definition based on https://infra.spec.whatwg.org/ spec & reform 'current high resolution time' to accept a global argument
igneel64 added a commit to igneel64/hr-time that referenced this issue Sep 22, 2020
Definition based on https://infra.spec.whatwg.org/ spec & reform 'current high resolution time' to accept a global argument
yoavweiss added a commit that referenced this issue Sep 22, 2020
Correct algorithm definition & 'current high resolution time' reform - Fixes #96
@yoavweiss yoavweiss reopened this Sep 22, 2020
@yoavweiss
Copy link
Contributor

Continuing discussion from #97 here:

  • "I think for DOM at least we wouldn't want to pass in a DOMHighResTimeStamp. We would want to pass something a time and a global (for the security check) and get back a DOMHighResTimeStamp to expose."
  • I replied
    • "I see a couple of places in DOM that call this:
      • "Initialize event’s timeStamp attribute to a DOMHighResTimeStamp representing the high resolution time from the time origin to now" - Would we be able to replace "now" with a call to "current high resolution time"?
      • "Let event be the result of running the inner event creation steps with this interface, null, now, and eventInitDict." - can we similarly replace "now" here?
      • "Let event be the result of running the inner event creation steps with eventInterface, realm, the time of the occurrence that the event is signaling, and dictionary" - "the time of the occurrence the event is signaling" seems fuzzier. Is there some other place to call "current high resolution time" that would signify that time?"
  • @annevk replied
    • "exactly the first place is where I'd rather have this specification do the whole thing if I ask for a relative high resolution time. E.g., "set timeStamp to a relative high resolution time object given time and global." or some such. And yeah, maybe "present time" can be a thing here that DOM can use instead of "now". The third bullet point is a bug in how dispatch events and the correct solution there is a lot of refactoring."

@yoavweiss
Copy link
Contributor

@annevk what we're aiming for the design here is to have a single place where jitter/rounding can be introduced, and as a result, a single place where these can be relaxed for "isolated" contexts (which can't load resources that are not CORP enabled).

Looking again, I think the first location in the DOM spec can simply call "current high resolution time".

It's just the third location that would have to be a bit more handwavy until a refactoring that defines the point in time that needs to be recorded is done. But given that it's already handwavy now, I'd prefer to make other callers more precise in the interim. Does that make sense?

@annevk
Copy link
Member Author

annevk commented Sep 23, 2020

Let's start with that I agree on the goals. I also appreciate you reopening this issue.

So I can see how most cases could be refactored to use "current high resolution time", which given a global object, returns a (potentially jittered/rounded) double.

However, the OS/kernel timestamp case cannot. There what we want, is an algorithm that given an external timestamp and a global, returns a (potentially jittered/rounded) double. Even in a future where we'd capture the OS timestamp at a more appropriate time (harhar).

It's also not entirely clear to me whether HTML's requestAnimationFrame() can use this as-is. Currently it relies on the event loop calling "current high resolution time", but the event loop spans an entire agent and is not bound to a particular global. So there too we have to capture "now" somehow and then later translate that into an appropriate (potentially jittered/rounded) double when there's a global in play.

So I still see the need for three primitives:

  • Unsafe now (a double that's not rounded or jittered and is for internal use only)
  • Current high resolution time (a double that's potentially rounded/jittered), which requires a global to be passed.
  • Relative high resolution time (a double that's potentially rounded/jittered), which requires a global and a double to be passed.

(And I guess there's the primitive of the timestamp on OS/kernel events, but that's really for various event specifications to formalize better.)

cc @domenic

@yoavweiss
Copy link
Contributor

That all sounds reasonable. I opened #99 to expose "unsafe now".

@yoavweiss
Copy link
Contributor

@annevk - I believe this is all properly handled now with unsafe shared current time, current hr-time, and relative hr-time.

WDYT? Can we close this?

@annevk
Copy link
Member Author

annevk commented Mar 9, 2021

Yeah, I suspect if there's follow-up that's better served with new issues at this point. Thanks for working on this!

@annevk annevk closed this as completed Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants