Skip to content

[v2] Add memory resource to owned and borrowed objects#419

Closed
Anilm3 wants to merge 0 commit into
anilm3/v2from
anilm3/v2-propagate-allocators
Closed

[v2] Add memory resource to owned and borrowed objects#419
Anilm3 wants to merge 0 commit into
anilm3/v2from
anilm3/v2-propagate-allocators

Conversation

@Anilm3

@Anilm3 Anilm3 commented Jun 18, 2025

Copy link
Copy Markdown
Collaborator

This PR expands #418 to use allocators on owned and borrowed objects, ensuring that objects are allocator-compatible and that all possible allocations are performed with the correct allocator.

All relevant changes are in object.hpp.

Note that the size of the PR is due to ancillary changes required due to the following changes to the owned object:

  • The nullptr_t constructor for owned_object has been removed as it was causing conflicts with other constructors.
  • The make_(array|map) functions accepting initializer lists have been extracted out into object_builder::(map|array) as they were also causing conflicts due to the introduction of the capacity parameter to the other overloads.

These changes can be safely ignored.

Remaining work (Next PRs):

  • Update objects to use allocators more effectively.
  • Check for allocator compatibility where relevant.
  • Propagate allocator from the context.
  • Have a clear distinction between input , output and internal allocators.
  • Update interface:
    • Pass input / ouput allocators to context_init.
    • Pass allocators to memory-allocating ddwaf_object functions.

@codecov-commenter

codecov-commenter commented Jun 18, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 64.86486% with 65 lines in your changes missing coverage. Please review.

Project coverage is 85.57%. Comparing base (29abeb0) to head (dd9fed2).

Files with missing lines Patch % Lines
src/object.hpp 65.29% 4 Missing and 55 partials ⚠️
src/memory_resource.hpp 42.85% 3 Missing and 1 partial ⚠️
src/processor/jwt_decode.cpp 50.00% 0 Missing and 1 partial ⚠️
src/serializer.cpp 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           anilm3/v2-use-allocators-internally     #419      +/-   ##
=======================================================================
- Coverage                                85.88%   85.57%   -0.31%     
=======================================================================
  Files                                      176      177       +1     
  Lines                                     9054     9138      +84     
  Branches                                  3853     3940      +87     
=======================================================================
+ Hits                                      7776     7820      +44     
+ Misses                                     498      497       -1     
- Partials                                   780      821      +41     
Flag Coverage Δ
waf_test 85.57% <64.86%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pr-commenter

pr-commenter Bot commented Jun 18, 2025

Copy link
Copy Markdown

Benchmarks clang

Benchmark execution time: 2025-06-20 23:06:34

Comparing candidate commit 907ed81 in PR branch anilm3/v2-propagate-allocators with baseline commit b94097a in branch anilm3/v2-use-allocators-internally.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 0 metrics, 0 unstable metrics.

scenario:global-benchmark.random.clang

  • 🟥 execution_time [+35.503ms; +35.637ms] or [+14.560%; +14.615%]

@pr-commenter

pr-commenter Bot commented Jun 18, 2025

Copy link
Copy Markdown

Benchmarks clang-pgo

Benchmark execution time: 2025-06-20 23:22:01

Comparing candidate commit 907ed81 in PR branch anilm3/v2-propagate-allocators with baseline commit b94097a in branch anilm3/v2-use-allocators-internally.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

@pr-commenter

pr-commenter Bot commented Jun 18, 2025

Copy link
Copy Markdown

Benchmarks gcc

Benchmark execution time: 2025-06-20 23:08:10

Comparing candidate commit 907ed81 in PR branch anilm3/v2-propagate-allocators with baseline commit b94097a in branch anilm3/v2-use-allocators-internally.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

@Anilm3
Anilm3 requested a review from Copilot June 19, 2025 15:32

This comment was marked as outdated.

@Anilm3
Anilm3 marked this pull request as ready for review June 20, 2025 09:54
@Anilm3
Anilm3 requested a review from a team as a code owner June 20, 2025 09:54
@github-actions

github-actions Bot commented Jul 2, 2025

Copy link
Copy Markdown

Artifact Size Comparison 📦

Artifact Previous Release This PR Difference
darwin-arm64::libddwaf.a 83315344 87218320 0.04%
darwin-arm64::libddwaf.a.stripped 4352184 4526504 0.04%
darwin-arm64::libddwaf.dylib 1919536 1934000 0.00%
darwin-universal::libddwaf.a 167911160 175906504 0.04%
darwin-universal::libddwaf.a.stripped 9268592 9647264 0.04%
darwin-universal::libddwaf.dylib 4033072 4063920 0.00%
darwin-x86_64::libddwaf.a 84595768 88688136 0.04%
darwin-x86_64::libddwaf.a.stripped 4916360 5120712 0.04%
darwin-x86_64::libddwaf.dylib 2091048 2105400 0.00%
linux-aarch64::libddwaf.a 68876962 70677778 0.02%
linux-aarch64::libddwaf.a.stripped 11720766 12010382 0.02%
linux-aarch64::libddwaf.so 2429048 2438680 0.00%
linux-armv7::libddwaf.a 60213732 61660844 0.02%
linux-armv7::libddwaf.a.stripped 10612116 10847488 0.02%
linux-armv7::libddwaf.so 2103320 2096504 0.00%
linux-i386::libddwaf.a 58414994 59867848 0.02%
linux-i386::libddwaf.a.stripped 9009642 9228764 0.02%
linux-i386::libddwaf.so 2302460 2308476 0.00%
linux-x86_64::libddwaf.a 68977050 70774866 0.02%
linux-x86_64::libddwaf.a.stripped 11389686 11656390 0.02%
linux-x86_64::libddwaf.so 2602216 2610048 0.00%
windows-win32::ddwaf.dll 3292160 3346432 0.01%
windows-win32::ddwaf.lib 11684 11938 0.02%
windows-win32::ddwaf_static.lib 46934870 48757834 0.03%
windows-x64::ddwaf.dll 4021248 4059648 0.00%
windows-x64::ddwaf.lib 11464 11712 0.02%
windows-x64::ddwaf_static.lib 54410908 56273738 0.03%

@Anilm3
Anilm3 requested a review from Copilot July 3, 2025 13:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors test code to consistently use the new object_builder API for map and array creation, ensures allocator compatibility in owned_object, and adds allocator-behavior tests in object_test.cpp.

  • Replaces owned_object::make_map, make_array, and raw nullptr constructors with object_builder::map, object_builder::array, and owned_object::make_null() across the test suite.
  • Updates integer literal suffixes (1UL1U) for initializer-list overloads.
  • Introduces memory_resource tests in tests/unit/object_test.cpp to verify allocation and deallocation behavior and allocator incompatibility.

Reviewed Changes

Copilot reviewed 40 out of 40 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/unit/waf_test.cpp Switched map creation to object_builder::map
tests/unit/value_iterator_test.cpp Replaced array/map creation with object_builder variants
tests/unit/rule_test.cpp Updated root object creation to object_builder::map
tests/unit/processor/structured_processor*.cpp Adjusted map/array builders and changed 1UL to 1U
tests/unit/processor/processor_test.cpp Switched map initializations to object_builder::map
tests/unit/processor/jwt_decode_test.cpp Replaced owned_object::make_map/array with object_builder calls
tests/unit/processor/fingerprint_test.cpp Updated map/array builders
tests/unit/object_view_test.cpp Switched to object_builder::map/array for nested object setup
tests/unit/object_test.cpp Added memory_resource tests, included allocator tests
tests/unit/module_test.cpp Replaced make_map calls with object_builder::map
tests/unit/kv_iterator_test.cpp Swapped to object_builder::array/map
tests/unit/key_iterator_test.cpp Updated array/map initializations to object_builder
tests/unit/expression_test.cpp Used object_builder::map for all root objects
tests/unit/exclusion/**/*.cpp Replaced all owned_object::make_map with object_builder::map

Base automatically changed from anilm3/v2-use-allocators-internally to anilm3/v2 July 4, 2025 14:05
@Anilm3 Anilm3 closed this Jul 4, 2025
@Anilm3
Anilm3 force-pushed the anilm3/v2-propagate-allocators branch from dd9fed2 to 154cbfe Compare July 4, 2025 14:07
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

Successfully merging this pull request may close these issues.

3 participants