Skip to content

Conversation

@pganssle
Copy link
Contributor

@pganssle pganssle commented Jan 5, 2018

Per my discussion in #171 (comment) and the same reasoning as arrow-py/arrow#486 (comment), I think that pendulum should take advantage of the fact that datetime.now() takes a tzinfo object as an argument to provide aware datetimes.

In Python 3.6 on my Arch Linux laptop. With this patch:

In [1]: from pendulum import Pendulum

In [2]: %timeit Pendulum.utcnow()
The slowest run took 4.25 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 11.4 µs per loop

In [3]: %timeit Pendulum.now('UTC')
The slowest run took 6.06 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 11.3 µs per loop

In [4]: %timeit Pendulum.now()
The slowest run took 628.80 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 18.7 µs per loop

Before this patch:

In [1]: from pendulum import Pendulum

In [2]: %timeit Pendulum.utcnow()
The slowest run took 4.17 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 11.1 µs per loop

In [3]: %timeit Pendulum.now('UTC')
100000 loops, best of 3: 11.1 µs per loop

In [4]: %timeit Pendulum.now()
The slowest run took 228.83 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 33 µs per loop

I guess it doesn't have a major effect on the speed of utcnow() for whatever reason, but it at the very least doesn't have a negative performance impact, and it's also the right thing to do when constructing aware datetimes.

@pganssle
Copy link
Contributor Author

pganssle commented Jan 5, 2018

There are also several places in the tests where you use utcnow() when now(UTC) would work just fine, but I didn't want to touch those because I was not entirely sure if constructing an instance from a naive datetime was actually part of the test for some reason.

It does seem to me that a lot of the tests use now() or utcnow() when they could use datetime literals instead, I'm not sure why that was done that way.

@sdispater sdispater merged commit f3a2cbe into python-pendulum:master Jan 19, 2018
@sdispater
Copy link
Collaborator

Thanks for that !

Regarding the use of now() or utcnow() in the tests, it's just for conciseness more than anything.

@pganssle pganssle deleted the remove_utcnow branch January 19, 2018 17:54
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.

2 participants