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

Memory usage of polyfill #683

Closed
jorroll opened this issue Jun 17, 2020 · 6 comments
Closed

Memory usage of polyfill #683

jorroll opened this issue Jun 17, 2020 · 6 comments
Labels
feedback later-production-polyfill non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!

Comments

@jorroll
Copy link

jorroll commented Jun 17, 2020

This issue is a follow up to #148

I maintain a javascript recurring dates library, rSchedule, which happens to be date library agnostic (i.e. it supports moment, luxon, js-joda, and standard Date). For fun, I decided to try implementing a date adapter for the temporal polyfill and running it against my tests (can be found in the temporal-date-adapter-2 branch).

Results

The good news is that integrating the current (experimental) version of the temporal polyfill into my library was easy and it successfully passed all of my 29,794 tests (my library has a custom date object that does all the heavy lifting, so these tests don't actually test proposal-temporal much).

  • The less great news is that the temporal polyfill required over 12gb of memory to complete all of these tests, while luxon uses about 1 gb of memory to pass the same test suite (not to mention the speed difference). Because the proposal-temporal polyfill is just intended for experimentation, this performance might be acceptible, but I wanted to pass along the info regardless.

Separately, one (small) usability issue I ran into:

Say you have an object like the following and you want to get an Absolute

interface IDateTimeArgument {
  timezone: string | null,
  year: number,
  month: number,
  day: number,
  hour: number,
  minute: number,
  second: number,
  millisecond: number,
}

You can do

declare const json: IDateTimeArgument;

Temporal.DateTime.from({
  year: json.year,
  month: json.month,
  day: json.day,
  hour: json.hour,
  minute: json.minute,
  second: json.second,
  millisecond: json.millisecond,  
}).inTimeZone(
  json.timezone === null ? Temporal.now.timeZone() : Temporal.TimeZone.from(json.timezone)
)

However you can't do the following, which was surprising and slightly annoying.

Temporal.now.timeZone().getAbsoluteFor({
  year: json.year,
  month: json.month,
  day: json.day,
  hour: json.hour,
  minute: json.minute,
  second: json.second,
  millisecond: json.millisecond,
})

It would be helpful if TimeZone#getAbsoluteFor() accepted a DateTimeLike object.

This issue was intended as feedback, so feel free to close.

@ryzokuken
Copy link
Member

ryzokuken commented Jun 17, 2020

@thefliik can't you just put the options bag inside a Temporal.DateTime.from call?

@jorroll
Copy link
Author

jorroll commented Jun 17, 2020

@ryzokuken yes, the following works:

Temporal.now.timeZone().getAbsoluteFor(
  Temporal.DateTime.from({
    year: json.year,
    month: json.month,
    day: json.day,
    hour: json.hour,
    minute: json.minute,
    second: json.second,
    millisecond: json.millisecond,  
  })
)

Though I find it surprising that it's necessary. Not clear why you can't just supply the options object directly as an argument for getAbsoluteFor(). But I can understand if others disagree.

@justingrant
Copy link
Collaborator

justingrant commented Jun 17, 2020

@thefliik - One reason is that the TimeZone class is intentionally more of an "advanced user" type. It's not optimized for ergonomics, partly to encourage mainstream use of types that are simpler to understand for developers who are less familiar with the intricacies of time zones.

There's also discussion about whether there should be a higher-level-of-abstraction type that combines a TimeZone and an Absolute into one object, and which would be optimized for ergonomics. If that type existed, then you'd write something like this:

Temporal.SomeNewType.from({
  timeZone: Temporal.now.timeZone(),
  year: json.year,
  month: json.month,
  day: json.day,
  hour: json.hour,
  minute: json.minute,
  second: json.second,
  millisecond: json.millisecond,  
})

I'm working on a prototype of this type and will share soon.

@ptomato
Copy link
Collaborator

ptomato commented Jun 18, 2020

@thefliik Thanks for taking the time to give feedback! Please, if you run into any more things, don't hesitate to open more issues here.

The usability issue is also being discussed in #592, could we move the discussion over there?

The less great news is that the temporal polyfill required over 12gb of memory to complete all of these tests, while luxon uses about 1 gb of memory to pass the same test suite (not to mention the speed difference). Because the proposal-temporal polyfill is just intended for experimentation, this performance might be acceptible, but I wanted to pass along the info regardless.

Agreed, I'd say this is acceptable unless it indicates that future implementations inside a real JS engine might have the same performance problems. That said, any PR on this topic would be welcome.

@ptomato
Copy link
Collaborator

ptomato commented Jun 24, 2020

I'll change the topic of this issue to reflect the memory performance since we are discussing the from() usability in #592.

@ptomato ptomato changed the title More feedback from my attempt to utilize the current API Memory usage of polyfill Jun 24, 2020
@cjtenny cjtenny added the non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! label Feb 18, 2021
@ptomato
Copy link
Collaborator

ptomato commented Sep 2, 2021

The polyfill in this repo has now been deprecated. This is a good thing for future polyfills to take into account, so I've copied part of the discussion over to js-temporal/temporal-polyfill#39

@ptomato ptomato closed this as completed Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback later-production-polyfill non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

No branches or pull requests

5 participants