Skip to content

Enable compression when sending json to client#11165

Merged
balloob merged 3 commits into
home-assistant:devfrom
elupus:elupus-patch-compress-json
Feb 18, 2018
Merged

Enable compression when sending json to client#11165
balloob merged 3 commits into
home-assistant:devfrom
elupus:elupus-patch-compress-json

Conversation

@elupus
Copy link
Copy Markdown
Contributor

@elupus elupus commented Dec 16, 2017

Make server compress json content when transmitting to client. Json is quite verbose and compresses well.

A real world example is history_graph requested data for in my case 4 temperature sensors updating every half a second for a graph over 10 days lead to 6MB json which compressed to 200KB using deflate compression.

Make server compress json content when transmitting to client. Json is quite verbose and compresses well.

A real world example is history_graph requested data for in my case 4 temperature sensors updating every half a second for a graph over 10 days lead to 6MB json which compressed to 200KB using deflate compression.
@elupus elupus requested a review from a team as a code owner December 16, 2017 13:00
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @elupus,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@elupus
Copy link
Copy Markdown
Contributor Author

elupus commented Dec 16, 2017

Note. This was a quick proof of concept from github gui, that was tested locally with just web tests.

@andrey-git
Copy link
Copy Markdown
Contributor

This needs on-device testing. Specifically how cpu-time compares between transmitting uncompressed data vs compressing+transmitting compressed data.

msg = json.dumps(
result, sort_keys=True, cls=rem.JSONEncoder).encode('UTF-8')
return web.Response(
j = web.Response(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preference:
Please name this response instead of j.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elupus FYI: you renamed it to request instead of response, as requested.

@Klathmon
Copy link
Copy Markdown
Contributor

Klathmon commented Jan 4, 2018

Even the most underpowered CPUs should be able to handle deflate without breaking a sweat.

I don't have any benchmarks on hand, but I did some testing on this a year or 2 ago when sending big JSON payloads from a raspberry pi and even over a local connection the gzipped content loaded significantly faster than the ungzipped content.

@elupus
Copy link
Copy Markdown
Contributor Author

elupus commented Jan 4, 2018 via email

@andrey-git
Copy link
Copy Markdown
Contributor

Kind ping :)

@rytilahti
Copy link
Copy Markdown
Member

I think this is fine, a client which cannot handle gzip compression should not send a header indicating it wants to accept such content. If that's the case, enable_compression() does not per default force compression. That being said, I agree with @andrey-git that it would be nice to have a benchmark in the test-suite for this!

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Jan 30, 2018

Benchmarks would be cool, but I don't think it's worth blocking a merge on it. We know that the JSON we're serving will dramatically benefit even from just gzip compression, and gzip is really lightweight to decompress. It's up to clients to signal their preferred compression formats.

@andrey-git
Copy link
Copy Markdown
Contributor

My concern was about server CPU, not client CPU.

@kennedyshead
Copy link
Copy Markdown
Contributor

I think that networkIO is worse than a couple of CPU-cycles, but it should be benchmarked.

AND this should be in mind https://en.wikipedia.org/wiki/BREACH

@Klathmon
Copy link
Copy Markdown
Contributor

Klathmon commented Jan 31, 2018

BREACH isn't an issue as we aren't compressing data from an attacker.

Basically, if an attacker could modify data being sent to craft a chosen-plaintext attack, they have already owned the system.

And about the benchmarks, would it be easier to merge this behind a flag, then after it is out in the wild ask for benchmarks from various users to determine if it should be enabled by default?

That will hit more users, use cases, and hardware than we would ever be able to test before releasing it, and it would give any users impacted the ability to undo this change quickly in their config.

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Jan 31, 2018

@andrey-git Ah, that does make more sense.

I'd still be inclined to turn this on as a more sane default, and ask for benchmarks to justify disabling it for specific use cases though.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 18, 2018

Let's turn it on and see how it goes.

@balloob balloob merged commit 92aeef8 into home-assistant:dev Feb 18, 2018
dmulcahey pushed a commit to dmulcahey/home-assistant that referenced this pull request Feb 19, 2018
* Enable compression when sending json to client

Make server compress json content when transmitting to client. Json is quite verbose and compresses well.

A real world example is history_graph requested data for in my case 4 temperature sensors updating every half a second for a graph over 10 days lead to 6MB json which compressed to 200KB using deflate compression.

* Rename variable to request

* Name the variable response instead of request
@balloob balloob mentioned this pull request Feb 22, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
@elupus elupus deleted the elupus-patch-compress-json branch May 2, 2019 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants