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

Restructure zone getter to allow for lazy unpacking. #216

Merged
merged 3 commits into from
May 30, 2015

Conversation

timrwood
Copy link
Member

Another alternative to #211 and #215.

This approach is much closer to the approach in #211, but cleans up a bunch of internals in the process.

We load data like below. This is the example at https://jsfiddle.net/rn58wmvn/3/.

moment.tz.load({
    version: "2015a",
    zones: [ "America/Chicago|CST CDT|60 50|01010101010101010101010|1BQU0 1zb0 Op0 1zb0 Op0 1zb0 Op0 1zb0 Op0 1zb0 Op0 1zb0 Rd0 1zb0 Op0 1zb0 Op0 1zb0 Op0 1zb0 Op0 1zb0"],
    links: ["America/Chicago|US/Central"]
});

The internal representation looks like this.

var links = {
  america_chicago: 'us_central',
  us_central: 'america_chicago'
};
var zones = {
  america_chicago: 'America/Chicago|CST CDT|60 50|0101010101010101...'
};
var names = {
  america_chicago: 'America/Chicago',
  us_central: 'US/Central'
};

Now we try to load the data for US/Central.

We check if there is an unpacked zone for us_central. There is not, so we continue.

We then check if there is a packed string for us_central. There is not, so we continue.

We then check if there is a link for us_central. There is a link, america_chicago, so we call getZone on america_chicago.

We check if there is an unpacked zone for america_chicago. There is not, so we continue.

We then check if there is a packed string for america_chicago. There is, so we unpack it. The data now looks like this.

var links = {
  america_chicago: 'us_central',
  us_central: 'america_chicago'
};
var zones = {
  america_chicago: Zone(name: 'America/Chicago', abbrs: ['CST', 'CDT', '...'])
};
var names = {
  america_chicago: 'America/Chicago',
  us_central: 'US/Central'
};

That zone is returned to the function looking for the us_central zone, and we use that and the names hash to populate the new zone. The data looks like this.

var links = {
  america_chicago: 'us_central',
  us_central: 'america_chicago'
};
var zones = {
  america_chicago: Zone(name: 'America/Chicago', abbrs: ['CST', 'CDT', '...'])
  us_central: Zone(name: 'US/Central', abbrs: ['CST', 'CDT', '...'])
};
var names = {
  america_chicago: 'America/Chicago',
  us_central: 'US/Central'
};

Subsequent calls to get the zone for America/Chicago and US/Central will now be short-circuted by the check for an unpacked zone.

The perf for this approach is roughly the same for the approach in #211.

This approach. http://jsperf.com/moment-timezone-unpack-on-demand/5
#211 approach. http://jsperf.com/moment-timezone-unpack-on-demand-2/2

I've only tested on a couple desktop browsers, so it would be good to see if the same perf improvements seen on android with @davidpean's implementation are applied here as well. cc @paulirish

`getZone` first checks if a zone is already parsed and returns it if so.
`getZone` first checks if there is a packed string available to use. If there is, it unpacks it and returns the new zone.
`getZone` then checks if there is link defined for that name. If there is, it calls `getZone` with the linked name and uses that to populate the data.
@davidpean
Copy link

@timrwood seeing a similar 25-50ms load time on my Android -- Nice!!

@paulirish
Copy link

Very nice. :D

timrwood added a commit that referenced this pull request May 30, 2015
Restructure zone getter to allow for lazy unpacking.
@timrwood timrwood merged commit d064a7e into moment:develop May 30, 2015
@timrwood timrwood deleted the unpack-on-demand-2 branch May 30, 2015 15:37
@timrwood timrwood mentioned this pull request May 30, 2015
@timrwood timrwood mentioned this pull request Sep 29, 2015
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 this pull request may close these issues.

3 participants