panthomakos / timezone

Accurate current and historical timezones for Ruby with support for Geonames and Google latitude - longitude lookups.
http://rubygems.org/gems/timezone
MIT License
354 stars 49 forks source link

`Marshal` dumps cached `@rules` #97

Closed nobu closed 5 years ago

nobu commented 6 years ago

Abstract

Marshal.dumped Timezone::Zone object contains cached @rules. It doesn't feel preferable and intended.

Example

Just created Timezone:

$ ruby -rtimezone -e 'tz = Timezone["America/New_York"]; Marshal.dump(tz, STDOUT)' | od -tx1z
0000000 04 08 6f 3a 13 54 69 6d 65 7a 6f 6e 65 3a 3a 5a  >..o:.Timezone::Z<
0000020 6f 6e 65 06 3a 0a 40 6e 61 6d 65 49 22 15 41 6d  >one.:.@nameI".Am<
0000040 65 72 69 63 61 2f 4e 65 77 5f 59 6f 72 6b 06 3a  >erica/New_York.:<
0000060 06 45 54                                         >.ET<
0000063

After using the rules once:

$ ruby -rtimezone -e 'tz = Timezone["America/New_York"]; tz.utc_to_local(Time.now.utc); Marshal.dump(tz, STDOUT)' | od -tx1z
0000000 04 08 6f 3a 13 54 69 6d 65 7a 6f 6e 65 3a 3a 5a  >..o:.Timezone::Z<
0000020 6f 6e 65 07 3a 0a 40 6e 61 6d 65 49 22 15 41 6d  >one.:.@nameI".Am<
0000040 65 72 69 63 61 2f 4e 65 77 5f 59 6f 72 6b 06 3a  >erica/New_York.:<
0000060 06 45 54 3a 0b 40 72 75 6c 65 73 5b 01 ef 5b 09  >.ET:.@rules[.?[.<
(snip)
0013020 81 81 49 22 08 45 53 54 06 3b 07 54 46 69 fe b0  >..I".EST.;.TFi??<
0013040 b9                                               >?<
0013041

Solutions

I'm thinking of a few ways.

define _dump or marshal_dump method.

This is simple, but has a backward incompatibility.

$ ruby -rtimezone -e 'class Timezone::Zone; def _dump(n); @name; end; end; Marshal.dump(Timezone["America/New_York"], STDOUT)' |
>> ruby -rtimezone -e 'p Marshal.load(STDIN)'
Traceback (most recent call last):
    1: from -e:1:in `<main>'
-e:1:in `load': class Timezone::Zone needs to have method `_load' (TypeError)
$ ruby -rtimezone -e 'class Timezone::Zone; def marshal_dump; @name; end; end; Marshal.dump(Timezone["America/New_York"], STDOUT)' |
>> ruby -rtimezone -e 'p Marshal.load(STDIN)'
Traceback (most recent call last):
    1: from -e:1:in `<main>'
-e:1:in `load': instance of Timezone::Zone needs to have method `marshal_load' (TypeError)

remove @rules totally

Remove the assignment to the variable and call Loader.load always, in Timezone::Zone#private_rules. It is just a cache, and also Timezone::Loader does.

This doesn't have no incompatibility perhaps, but may have a (very) little overhead.

move @rules to Timezone module's instance variable

Similar to the previous one, but keep the 2-layer caches.

do not mind

It's just several KB :P

panthomakos commented 5 years ago

I finally got around to this. I like your idea of simply removing @rules. To avoid the performance hit I'm storing the rules in a local variable when I need them, and I have improved the performance of Loader::load so that it only uses a single Hash lookup.