search

project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.56k stars 2.04k forks source link

[ReadHandler] Reduce RAM usage #28277

Open lpbeliveau-silabs opened 1 year ago

lpbeliveau-silabs commented 1 year ago

The new implementation increased the RAM usage by more than 1k.

We need to investigate if we can reduce this number. Possible ways would be to replace the sync timestamp (uint64t) by a syncflag and modify the logic so it can serve the same purpose. (issue : https://github.com/project-chip/connectedhomeip/issues/28052)

Addressing issue: https://github.com/project-chip/connectedhomeip/issues/27920 might also reduce the RAM usage.

Addressing issue: https://github.com/project-chip/connectedhomeip/issues/28129 will also reduce the RAM usage.

bzbarsky-apple commented 1 year ago

Another thing we could conceivably do: right now each read handler node stores three timestamps (min, max, sync). That's 12 bytes, right? We could store min and two 16-bit offsets, which would be 8 bytes.

But the real question is: why 1KB? How many entries does the pool have? How big is each read handler node?

lpbeliveau-silabs commented 1 year ago

Even wort, timestamps are uint64_t, so each one is taking 8 bytes, so 24bytes worth of timestamp.

There is some logic that validates if max is greater or smaller than min and vice versa so storing the 16-bit offsets might be a bit complicated, that could be tackled in either https://github.com/project-chip/connectedhomeip/issues/27996 or https://github.com/project-chip/connectedhomeip/issues/27997 though.

For the sync timestamp, I am hoping to trade the sync timestamp for a uint8_t flag that would hold mEngineRunScheduled flag and a new Sync flag. That would require some rework on the sync logic, which is already needed in https://github.com/project-chip/connectedhomeip/issues/28083.

Regarding the size, given that the pool size is dictated by the maximum number of read handler: CHIP_IM_MAX_NUM_READS + CHIP_IM_MAX_NUM_SUBSCRIPTIONS , which defaults to 64, reducing the size of nodes has a huge impact.

bzbarsky-apple commented 1 year ago

timestamps are uint64_t

Ah, right, we store TimeStamp, not Timeout.

which defaults to 64

Wait, why does that default to 64? I would expect it, on any sort of constrained platform, to be 5 reads and 15 subscriptions, total of 20. Still pretty large if we have 24 bytes just for the timestamps...

lpbeliveau-silabs commented 1 year ago

Good news is with https://github.com/project-chip/connectedhomeip/pull/28733, we will only have 2 timestamps.

Wait, why does that default to 64?

It boils down to CHIP_CONFIG_MAX_FABRICS + CHIP_CONFIG_MAX_FABRICS 3, which is 16 + 16 3 from our default define in CHIPConfig.h if not defined previously.

It seems a lot of platform do define CHIP_CONFIG_MAX_FABRICS to 5, but this value changes in a per platform and per project basis.

bzbarsky-apple commented 1 year ago

which is 16 + 16 * 3 from our default define in CHIPConfig.h

None of the constrained devices use that. What's the actual RAM use on an actual device, not linux/mac, where no one is going to care about a few KB of RAM?