Skip to content

Commit

Permalink
Reduce use of maximum-sized packet buffers (project-chip#4434)
Browse files Browse the repository at this point in the history 
* Reduce use of maximum-sized packet buffers

CHIP code contains many uses of PacketBuffer::New(), which allocates a
maximum-size buffer. This is often much larger than actually required,
which wastes heap.

- Replaced `PacketBuffer::New()` and `NewWithAvailableSize()` with
  `PacketBufferHandle::New()`, which requires an explicit packet size.
  (The parameter order is opposite to the old `NewWithAvailableSize()`
  because the reserved size is usually the default.) Made the available
  parameter a `size_t` because for many callers it originates as such,
  and `New()` already checks it; this allows removing `CanCastTo` tests
  from callers. Also, documented the guarantee that `New()` never
  returns a buffer with less space than requested, and removed post-New
  size tests.
- Added `NewWithData()` to encapsulate a common use of `New()`, and
  reduce the risk of length errors between the steps.
- Implemented `RightSize()` for the heap allocation case.
- Added constants `kMaxPacketBufferSizeWithoutReserve` and
  `kMaxPacketBufferSize` for use where the maximum size is required or
  the size is unknown.
- Added `kMaxIPAddressStringLength` for `NetworkProvisioning::SendIPAddress`.
- Added `NetworkProvisioning::EncodedStringSize()` for `SendNetworkCredentials()`.
- Moved some size constants into BLE header files.

fixes project-chip#4351 - Reduce use of maximum-sized packet buffers

* Fix merge of remote-tracking branch 'origin/master' into x4351-new

* review

* restyled
  • Loading branch information
@kpschoedel @austinh0
kpschoedel authored and austinh0 committed Feb 3, 2021
1 parent 01ac6e9 commit 9227a77
Show file tree
Hide file tree
Showing 52 changed files with 563 additions and 561 deletions.
4 changes: 2 additions & 2 deletions examples/minimal-mdns/client.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -250,7 +250,7 @@ class QuerySplitter

void BroadcastPacket(mdns::Minimal::ServerBase * server)
{
System::PacketBufferHandle buffer = System::PacketBuffer::NewWithAvailableSize(kMdnsMaxPacketSize);
System::PacketBufferHandle buffer = System::PacketBufferHandle::New(kMdnsMaxPacketSize);
if (buffer.IsNull())
{
printf("Buffer allocation failure.");
Expand Down
6 changes: 3 additions & 3 deletions src/app/Command.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2021 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -58,7 +58,7 @@ CHIP_ERROR Command::Reset()
if (mCommandMessageBuf.IsNull())
{
// TODO: Calculate the packet buffer size
mCommandMessageBuf = System::PacketBuffer::New();
mCommandMessageBuf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize);
VerifyOrExit(!mCommandMessageBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY);
}

Expand Down Expand Up @@ -145,7 +145,7 @@ void Command::Shutdown()

chip::TLV::TLVWriter & Command::CreateCommandDataElementTLVWriter()
{
mCommandDataBuf = chip::System::PacketBuffer::New();
mCommandDataBuf = chip::System::PacketBufferHandle::New(System::kMaxPacketBufferSize);
if (mCommandDataBuf.IsNull())
{
ChipLogDetail(DataManagement, "Unable to allocate packet buffer");
Expand Down
4 changes: 2 additions & 2 deletions src/app/encoder.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -74,7 +74,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
#define COMMAND_HEADER(name, clusterId) \
const char * kName = name; \
\
PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize); \
PacketBufferHandle payload = PacketBufferHandle::New(kMaxBufferSize); \
if (payload.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
Expand Down
36 changes: 18 additions & 18 deletions src/app/tests/TestMessageDef.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ void AttributePathTest(nlTestSuite * apSuite, void * apContext)
AttributePath::Builder attributePathBuilder;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
attributePathBuilder.Init(&writer);
BuildAttributePath(apSuite, attributePathBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -772,7 +772,7 @@ void AttributePathListTest(nlTestSuite * apSuite, void * apContext)
chip::System::PacketBufferTLVReader reader;
AttributePathList::Builder attributePathListBuilder;

writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));

err = attributePathListBuilder.Init(&writer);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -797,7 +797,7 @@ void EventPathTest(nlTestSuite * apSuite, void * apContext)
EventPath::Builder eventPathBuilder;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
eventPathBuilder.Init(&writer);
BuildEventPath(apSuite, eventPathBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -821,7 +821,7 @@ void EventPathListTest(nlTestSuite * apSuite, void * apContext)
chip::System::PacketBufferTLVReader reader;
EventPathList::Builder eventPathListBuilder;

writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));

err = eventPathListBuilder.Init(&writer);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -845,7 +845,7 @@ void CommandPathTest(nlTestSuite * apSuite, void * apContext)
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
CommandPath::Builder commandPathBuilder;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
err = commandPathBuilder.Init(&writer);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

Expand All @@ -871,7 +871,7 @@ void EventDataElementTest(nlTestSuite * apSuite, void * apContext)
EventDataElement::Parser eventDataElementParser;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
eventDataElementBuilder.Init(&writer);
BuildEventDataElement(apSuite, eventDataElementBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -894,7 +894,7 @@ void EventListTest(nlTestSuite * apSuite, void * apContext)
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
EventList::Builder eventListBuilder;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
eventListBuilder.Init(&writer);
BuildEventList(apSuite, eventListBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -916,7 +916,7 @@ void StatusElementTest(nlTestSuite * apSuite, void * apContext)
StatusElement::Parser statusElementParser;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
statusElementBuilder.Init(&writer);
BuildStatusElement(apSuite, statusElementBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -940,7 +940,7 @@ void AttributeStatusElementTest(nlTestSuite * apSuite, void * apContext)
AttributeStatusElement::Parser attributeStatusElementParser;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
attributeStatusElementBuilder.Init(&writer);
BuildAttributeStatusElement(apSuite, attributeStatusElementBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -962,7 +962,7 @@ void AttributeStatusListTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
AttributeStatusList::Builder attributeStatusListBuilder;
err = attributeStatusListBuilder.Init(&writer);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand All @@ -986,7 +986,7 @@ void AttributeDataElementTest(nlTestSuite * apSuite, void * apContext)
AttributeDataElement::Parser attributeDataElementParser;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
attributeDataElementBuilder.Init(&writer);
BuildAttributeDataElement(apSuite, attributeDataElementBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -1008,7 +1008,7 @@ void AttributeDataListTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
AttributeDataList::Builder attributeDataListBuilder;
attributeDataListBuilder.Init(&writer);
BuildAttributeDataList(apSuite, attributeDataListBuilder);
Expand All @@ -1029,7 +1029,7 @@ void AttributeDataVersionListTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
AttributeDataVersionList::Builder attributeDataVersionListBuilder;
attributeDataVersionListBuilder.Init(&writer);
BuildAttributeDataVersionList(apSuite, attributeDataVersionListBuilder);
Expand All @@ -1052,7 +1052,7 @@ void CommandDataElementTest(nlTestSuite * apSuite, void * apContext)
CommandDataElement::Parser commandDataElementParser;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
commandDataElementBuilder.Init(&writer);
BuildCommandDataElement(apSuite, commandDataElementBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -1075,7 +1075,7 @@ void CommandListTest(nlTestSuite * apSuite, void * apContext)
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
CommandList::Builder commandListBuilder;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
commandListBuilder.Init(&writer);
BuildCommandList(apSuite, commandListBuilder);
chip::System::PacketBufferHandle buf;
Expand All @@ -1095,7 +1095,7 @@ void ReportDataTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
BuildReportData(apSuite, writer);
chip::System::PacketBufferHandle buf;
err = writer.Finalize(&buf);
Expand All @@ -1114,7 +1114,7 @@ void InvokeCommandTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
BuildInvokeCommand(apSuite, writer);
chip::System::PacketBufferHandle buf;
err = writer.Finalize(&buf);
Expand All @@ -1133,7 +1133,7 @@ void ReadRequestTest(nlTestSuite * apSuite, void * apContext)
CHIP_ERROR err = CHIP_NO_ERROR;
chip::System::PacketBufferTLVWriter writer;
chip::System::PacketBufferTLVReader reader;
writer.Init(chip::System::PacketBuffer::New());
writer.Init(chip::System::PacketBufferHandle::New(chip::System::kMaxPacketBufferSize));
BuildReadRequest(apSuite, writer);
chip::System::PacketBufferHandle buf;
err = writer.Finalize(&buf);
Expand Down
4 changes: 2 additions & 2 deletions src/app/util/chip-message-send.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2021 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -55,7 +55,7 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
return EMBER_ERR_FATAL;
}

System::PacketBufferHandle buffer = System::PacketBuffer::NewWithAvailableSize(dataLength);
System::PacketBufferHandle buffer = System::PacketBufferHandle::New(dataLength);
if (buffer.IsNull())
{
// FIXME: Not quite right... what's the right way to indicate "out of
Expand Down
2 changes: 1 addition & 1 deletion src/app/zap-templates/templates/chip/encoder-src.zapt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
#define COMMAND_HEADER(name, clusterId) \
const char * kName = name; \
\
PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize); \
PacketBufferHandle payload = PacketBufferHandle::New(kMaxBufferSize); \
if (payload.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
Expand Down
8 changes: 4 additions & 4 deletions src/ble/BLEEndPoint.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
* Copyright (c) 2020-2021 Project CHIP Authors
* Copyright (c) 2014-2017 Nest Labs, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -115,7 +115,7 @@ BLE_ERROR BLEEndPoint::StartConnect()
mState = kState_Connecting;

// Build BLE transport protocol capabilities request.
buf = System::PacketBuffer::New();
buf = System::PacketBufferHandle::New(System::kMaxPacketBufferSize);
VerifyOrExit(!buf.IsNull(), err = BLE_ERROR_NO_MEMORY);

// Zero-initialize BLE transport capabilities request.
Expand Down Expand Up @@ -945,7 +945,7 @@ BLE_ERROR BLEEndPoint::DriveStandAloneAck()
// If stand-alone ack not already pending, allocate new payload buffer here.
if (mAckToSend.IsNull())
{
mAckToSend = System::PacketBuffer::New();
mAckToSend = System::PacketBufferHandle::New(kTransferProtocolStandaloneAckHeaderSize);
VerifyOrExit(!mAckToSend.IsNull(), err = BLE_ERROR_NO_MEMORY);
}

Expand Down Expand Up @@ -1087,7 +1087,7 @@ BLE_ERROR BLEEndPoint::HandleCapabilitiesRequestReceived(PacketBufferHandle data
err = BleTransportCapabilitiesRequestMessage::Decode(data, req);
SuccessOrExit(err);

responseBuf = System::PacketBuffer::New();
responseBuf = System::PacketBufferHandle::New(kCapabilitiesResponseLength);
VerifyOrExit(!responseBuf.IsNull(), err = BLE_ERROR_NO_MEMORY);

// Determine BLE connection's negotiated ATT MTU, if possible.
Expand Down
Loading

0 comments on commit 9227a77

Please sign in to comment.