search

readium / readium-sdk

A C++ ePub renderer SDK
BSD 3-Clause "New" or "Revised" License
385 stars 164 forks source link

Adds HTTP caching headers to responses in iOS RDServices #235

Closed olivierkorner closed 8 years ago

olivierkorner commented 8 years ago

it should provide a 10-30% speed boost to page rendering.

rkwright commented 8 years ago

I am in favor of merging this. My primary concern is how we can measure the change and how to determine that there are no downsides to it.

olivierkorner commented 8 years ago

@rkwright I measured the time between the call to openPage and the moment the PAGINATION_CHANGED is emitted, in ReaderView, using the function window.performance.now. I displayed the 20 first pages of a content heavy book (many images, fonts...), measuring the rendering time for each page, without and with caching headers.

rkwright commented 8 years ago

OK, thanks. I did see your data in the issue. I guess we'll just have to watch out for any unexpected behaviour that might result from the changes. But the upside is excellent. Thanks for doing this.

@danielweck - if you think the change is good, I am fine with your merging it into develop.

danielweck commented 8 years ago

Thanks for addressing my minor concern about the duplicated / unused code block :)

This PR needs to target the develop branch though (not master).

danielweck commented 8 years ago

Unfortunately the PR needs to be recreated in order to change the target branch from master to develop. I don't know any other way.

danielweck commented 8 years ago

By the way @olivierkorner thanks a lot for doing the analysis + implementation, this is a great performance trick :)

And just so you know: the epubReadingSystem script injection uses different "virtual" paths at each HTTP request, so that the returned payload never gets cached. That's because the HTTP request must always be intercepted for the injection to work. See m_epubReadingSystem_Counter in https://github.com/readium/readium-sdk/blob/develop/Platform/Apple/RDServices/Main/RDPackageResourceConnection.m#L40