Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReadHandler] Reduce RAM usage #28277

Open
lpbeliveau-silabs opened this issue Jul 25, 2023 · 5 comments
Open

[ReadHandler] Reduce RAM usage #28277

lpbeliveau-silabs opened this issue Jul 25, 2023 · 5 comments

Comments

@lpbeliveau-silabs
Copy link
Contributor

lpbeliveau-silabs commented Jul 25, 2023

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 : #28052)

Addressing issue: #27920 might also reduce the RAM usage.

Addressing issue: #28129 will also reduce the RAM usage.

@bzbarsky-apple
Copy link
Contributor

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
Copy link
Contributor Author

lpbeliveau-silabs commented Aug 3, 2023

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 #27996 or #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 #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
Copy link
Contributor

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
Copy link
Contributor Author

lpbeliveau-silabs commented Aug 17, 2023

Good news is with #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
Copy link
Contributor

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?

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

No branches or pull requests

2 participants