#49097 Pblock performance improvement
Closed: fixed 3 years ago Opened 3 years ago by firstyear.

The pblock is a large complex structure. We can improve the performance of the server as a whole if we improve our design and usage of this complex structure.

http://www.port389.org/docs/389ds/design/pblock-breakup.html

This can be targeted for 1.3.7 or later.


Metadata Update from @firstyear:
- Issue assigned to firstyear
- Issue set to the milestone: 1.3.7 backlog

3 years ago

Metadata Update from @firstyear:
- Issue close_status updated to: None
- Issue tagged with: Complex, Performance

3 years ago

0001-Ticket-49097-Pblock-get-set-cleanup.patch

This change, along with the fixes in #49222 passes all ticket tests with 100% success.

Metadata Update from @firstyear:
- Custom field reviewstatus adjusted to review

3 years ago

PS: Before you say it Mark, I think the patches to re-tab the source tree are coming in the next week, so please don't be strict on my tab vs spaces :)

PS: Before you say it Mark, I think the patches to re-tab the source tree are coming in the next week, so please don't be strict on my tab vs spaces :)

Yeah man it's bad. Java style comments, inconsistent spacing/tabs. The patch itself looks good, but I'm not giving an ack until you provide the patch to fix all the spacing/tab/comment issues.

Also did you do any testing to see how the server process size looks (before and after applying your patch)? And what about a sanity ASAN test?

Thanks!

Java style comments are easy to fix, whitespace, no so much. There is a lot of change in there...

I haven't re-shaped the pblock yet, so I expect memory usage is the same still. Also, my builds are always asan, so this 100% passes ticket tests with asan enabled the entire duration. I went hardcore on this one :)

How would you consider a second patch on top of this that fixes the whitespacing?

How would you consider a second patch on top of this that fixes the whitespacing?

A second patch is fine, but I want them committed at the same time. And what I really want is the tab vs space inconsistencies fixed. For example, in some places you add code that uses tabs, but the surrounding code uses whites spaces, or you use white spaces when the surrounding code uses tabs. So you are technically adding to the problems we have in source code. I don't want to keep making the problem worse.

So I'll keep an eye out for your new patch. Seriously, it should only take you 10 or15 minutes to fix it up.

Yep, I was planning to merge both at the same time. I'll start on this now :) I just prefer to keep whitespace fixes seperate to "code fixes" is all.

TBH, Is it time we just ran clangformat and fixed this to spaces to end the problem once and for all? This is why I was asking about branching in 389-devel.

Thanks again mate :)

0001-Ticket-49097-whitespace-fixes-for-pblock-change.patch

This patch applies "in addition to" and fixes the white space issues. If you want, I can squash the two together for the merge?

Thank you William! You don't need to squash them - two commits is fine.

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to ack (was: review)

3 years ago

commit 74c666b
commit eacb3c9
To ssh://git@pagure.io/389-ds-base.git
5680cc1..eacb3c9 master -> master

0001-Ticket-49097-fix-the-pblock-to-be-a-hierachial-struc.patch

This is the final patch in the series. This converts the pblock to a tree of structs, that are allocated as needed.

Heap profiling shows the output:

Before:
bytes allocated in total (ignoring deallocations): 436.90MB (3.41MB/s)
After:
bytes allocated in total (ignoring deallocations): 374.80MB (2.87MB/s)

This is a 15% reduction in heap allocations, and the maximum size of any structure in the tree drops from 712 bytes to 200 bytes (slapi_pblock_intop) with the root pblock now being 88 bytes. There is some room for further refinement especially in the internal search use case. This may have significant effects on reducing our memory fragmentation.

Like other patches in the series, this has been run with ASAN and test cases. Most importantly, was the psearch test case to prevent regressions there (as I previously introduced).

PS: @mreynolds there is a second patch to follow to re-whitespace the file.

Metadata Update from @firstyear:
- Custom field reviewstatus adjusted to review (was: ack)

3 years ago

And the second patch that fixes the whitespace

0001-Ticket-49097-fix-pblock-whitespace.patch

Metadata Update from @mreynolds:
- Custom field reviewstatus adjusted to ack (was: review)

3 years ago

commit 407d7d9
commit 7995007
To ssh://git@pagure.io/389-ds-base.git
a96c91d..7995007 master -> master

Metadata Update from @firstyear:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

3 years ago

Login to comment on this ticket.

Metadata