Skip to content

performance improvements in ACE ndr pushing and pulling

The addition of conditional ACEs complicated the NDR serialisation and parsing of ACEs, to the point that it more than doubled the time taken to process a security descriptor (on a not very modern CPU, "Dual Core AMD Opteron(tm) Processor 285" from 2009).

The x axis in the graph below shows commits in temporal order. First is 4.19, second is a origin/master, and from there are selected commits from this MR, though the hashes have changed through a rebase. The y-axis is relative time, compared to 4.19 (or master for the conditional ACE test, because it doesn't work in 4.19).

results-variance-over-0.005.svg

In 4.19 (and forever) we had manual ACE pull code that discard any trailing bytes (that is, when the ACE size field pointed beyond the end of the bytes that were actually used in the ACE). With the advent of conditional and RA ACEs, these trailing bytes had a meaning and had to be pulled into a blob. The easy thing to do then was to make an ignored blob for the trailing bytes of ordinary ACEs. This meant we could do away with the manual code. Unfortunately it also meant the performance was crap, because the trailing blob requires a subcontext with its own token lists and so forth, and there is a lot of tallocing to do nothing. It also changed the behaviour in the case of a round trip -- while the ignored blob is ignored within Samba, it gets serialised again in an ndr_push, which would previously not have been the case.

With this MR we bring the ACE push and pull code back into manual control, and short circuit where we know we don't want to do anything. But first there are a few patches for the performance code.

Other test runs show not much change between 4.12 and 4.19.

The test framework and graph code is at https://gitlab.com/samba-team/gitlab-runner/samba-cloud-autobuild/-/tree/douglas-py3

Checklist

  • Commits have Signed-off-by: with name/author being identical to the commit author
  • (optional) This MR is just one part towards a larger feature.
  • (optional, if backport required) Bugzilla bug filed and BUG: tag added
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated
  • CI timeout is 3h or higher (see Settings/CICD/General pipelines/ Timeout)

Reviewer's checklist:

  • There is a test suite reasonably covering new functionality or modifications
  • Function naming, parameters, return values, types, etc., are consistent and according to README.Coding.md
  • This feature/change has adequate documentation added
  • No obvious mistakes in the code

Merge request reports

Loading