Write a constant value to a file without buffering #2874
Conversation
…kes in memory consumption. Signed-off-by: Haruki Imai <[email protected]>
Signed-off-by: Haruki Imai <[email protected]>
Signed-off-by: Haruki Imai <[email protected]>
|
It looks good, I was interested to see if using mmap would be faster that the Where tests were done for 128B, 1KB, 256KB, 1MB, 10MB, and 100MB. For all but the smallest sizes, We can go with this PR as of now. It would be interesting to see what are the average size of the constant being written (maybe add a printf and run it through the large benchmark), and let see if might be worthwhile to buffer the writes using mmap. |
|
@AlexandreEichenberger Thanks for you suggestion! This PR focuses on peak memory reduction. May I use/try mmap in a follow-up PR focused on performance? |
For sure. When I saw your PR, I just got curious. |
| std::vector<char> packedConst; | ||
| llvm::sys::fs::remove(filepath); | ||
| std::ofstream outfile(filepath, std::ios::app | std::ios::binary); | ||
| int64_t totalConstSize = 0; |
There was a problem hiding this comment.
uint64_t is more suitable.
| for (int64_t i = globalOfInterest.size() - 1; i >= 0; --i) { | ||
| std::vector<char> packedConst; |
There was a problem hiding this comment.
The name packedConst is obsoleted in the new context since we donnot pack constants anymore. Maybe change it to paddedConstant or it can be totally removed (See the below comment)
| ((uint64_t)(packedConst.size() / alignment) + 1) * alignment - | ||
| packedConst.size(); | ||
| ((uint64_t)(totalConstSize / alignment) + 1) * alignment - | ||
| totalConstSize; | ||
| SmallVector<char> pads(padSize, (char)0); | ||
| packedConst.insert(packedConst.end(), pads.begin(), pads.end()); |
There was a problem hiding this comment.
We can reduce memory more by not using a padded buffer but two writes to a file: one for padding and one for rawData. For example.
// Padding the current data for correct alignment.
uint64_t padSize = 0;
if ((alignment > 0) && (totalConstSize % alignment != 0)) {
padSize = ((uint64_t)(totalConstSize / alignment) + 1) * alignment - totalConstSize;
SmallVector<char> pads(padSize, (char)0);
outfile.write(pads.data(), pads.size());
}
// Write the constant to a file.
uint64_t constSize = rawData.size();
outfile.write(rawData.data(), constSize);
// Update the total size and op's offset.
totalConstSize += padSize + constSize;
op.setOffsetAttr(b.getI64IntegerAttr(totalConstSize));
op.removeValueAttr();
There was a problem hiding this comment.
Smart, please do that.
There was a problem hiding this comment.
Thanks for your comments! I updated.
totalConstSize in op.setOffsetAttr(b.getI64IntegerAttr(totalConstSize)) is offset. So the totalConstSize before adding constSize should be used.
Signed-off-by: Haruki Imai <[email protected]>
Signed-off-by: Haruki Imai <[email protected]>
|
Jenkins Linux s390x Build #15178 [push] Write a constant value t... started at 22:46 |
|
Jenkins Linux ppc64le Build #14203 [push] Write a constant value t... started at 22:58 |
|
Jenkins Linux amd64 Build #15173 [push] Write a constant value t... started at 21:46 |
|
Jenkins Linux amd64 Build #15173 [push] Write a constant value t... passed after 1 hr 5 min |
|
Jenkins Linux s390x Build #15178 [push] Write a constant value t... passed after 1 hr 28 min |
|
Jenkins Linux ppc64le Build #14203 [push] Write a constant value t... passed after 1 hr 42 min |
This PR partially solves an issue of memory consumption reported in #2821
We found that there is a spike in memory consumption when writing a constant into a file (
model.constants.bin). This is because all constants into a buffer once and write it to a file at once. This PR changes to write the constant to the file without the buffering. This removes the spike in memory consumption.