search

apache / celix

Apache Celix is a framework for C and C++14 to develop dynamic modular software applications using component and in-process service-oriented programming.
https://celix.apache.org/
Apache License 2.0
158 stars 85 forks source link

Feature/674 use properties type in filter #692

Closed pnoltes closed 6 months ago

pnoltes commented 7 months ago

Intro

This PR updates the Apache Celix filter to utilize typed properties entries and also refactors the filter implementation.

My intention was to create a small PR allowing filters to work with typed properties before starting on array support. However, I observed several issues with the filter implementation that I also wanted to address.

Benchmark

With this PR, a filter match will take into account the underlying properties entry type and use it for a more efficient comparison. A Filter match benchmark is introduced (and run before using the typed properties entries in filter match) to ensure the desired effect.

Here are some before and after results (measured on my machine), where the range indicates the size of the properties set used within a filter match.

Before Refactor

--------------------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                            Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------------------------------------------------------------
FilterBenchmark_testStringCFilter/10000/process_time/real_time                    14.5 ns         14.5 ns     48853882 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=68.9608M/s
FilterBenchmark_testStringFilter/10000/process_time/real_time                     14.4 ns         14.4 ns     48922109 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=69.5677M/s
FilterBenchmark_testLongFilter/10000/process_time/real_time                       22.3 ns         22.3 ns     31418137 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=44.8544M/s
FilterBenchmark_testBoolFilter/10000/process_time/real_time                       16.3 ns         16.3 ns     42884878 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=61.21M/s
FilterBenchmark_testVersionFilter/10000/process_time/real_time                     131 ns          131 ns      5332730 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=7.6231M/s
FilterBenchmark_testVersionWithQualifierFilter/10000/process_time/real_time        163 ns          163 ns      4351054 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=6.13215M/s
FilterBenchmark_testStringGreaterEqualFilter/10000/process_time/real_time         14.4 ns         14.4 ns     48966825 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=69.4911M/s
FilterBenchmark_testLongGreaterEqualFilter/10000/process_time/real_time           22.1 ns         22.1 ns     29620437 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=45.307M/s
FilterBenchmark_testDoubleGreaterEqualFilter/10000/process_time/real_time         43.5 ns         43.5 ns     16141792 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=22.968M/s
FilterBenchmark_versionGreaterEqualFilter/10000/process_time/real_time             129 ns          129 ns      5429183 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=7.74544M/s
FilterBenchmark_versionRangeFilter/10000/process_time/real_time                    276 ns          276 ns      2534677 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=3.61734M/s
FilterBenchmark_substringFilter/10000/process_time/real_time                       276 ns          276 ns      2534366 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=3.61947M/s
FilterBenchmark_complexFilter/10000/process_time/real_time                        28.1 ns         28.1 ns     25516281 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=35.631M/s

After refactor

--------------------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                            Time             CPU   Iterations UserCounters...
--------------------------------------------------------------------------------------------------------------------------------------
FilterBenchmark_testStringCFilter/10000/process_time/real_time                    15.4 ns         15.4 ns     45535697 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=64.9322M/s
FilterBenchmark_testStringFilter/10000/process_time/real_time                     15.4 ns         15.4 ns     45357990 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=64.7906M/s
FilterBenchmark_testLongFilter/10000/process_time/real_time                       14.9 ns         14.9 ns     46921381 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=66.9823M/s
FilterBenchmark_testBoolFilter/10000/process_time/real_time                       15.4 ns         15.4 ns     45496705 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=64.8934M/s
FilterBenchmark_testVersionFilter/10000/process_time/real_time                    19.1 ns         19.1 ns     36715526 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=52.4212M/s
FilterBenchmark_testVersionWithQualifierFilter/10000/process_time/real_time       19.6 ns         19.6 ns     35566450 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=50.8923M/s
FilterBenchmark_testStringGreaterEqualFilter/10000/process_time/real_time         14.8 ns         14.8 ns     47205823 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=67.484M/s
FilterBenchmark_testLongGreaterEqualFilter/10000/process_time/real_time           15.7 ns         15.7 ns     44793529 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=63.8461M/s
FilterBenchmark_testDoubleGreaterEqualFilter/10000/process_time/real_time         16.7 ns         16.7 ns     42395112 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=60.0551M/s
FilterBenchmark_versionGreaterEqualFilter/10000/process_time/real_time            25.6 ns         25.6 ns     27321443 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=39.0526M/s
FilterBenchmark_versionRangeFilter/10000/process_time/real_time                   40.9 ns         40.9 ns     17123791 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=24.4385M/s
FilterBenchmark_substringFilter/10000/process_time/real_time                      40.8 ns         40.8 ns     17131224 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=24.5143M/s
FilterBenchmark_complexFilter/10000/process_time/real_time                        29.3 ns         29.3 ns     23892366 fillEntriesOptimizationBufferPercentage=1 fillStringOptimizationBufferPercentage=0.992188 items_per_second=34.1668M/s

Substring Refactor

The filter substring implementation was incorrect (not according to the RFC) and has been updated. Additionally, the previous implementation required a malloc during matching; this need has now been removed, ensuring that a filter match does not require additional memory and thus cannot fail due to an allocation error.

Small Refactor of AND and OR

The matching for AND and OR operators has been updated so that (|) and (&) will return true.

Service Registration

Service registration, including component service registration, has been updated. Now, service.id and service.version are stored as long and celix_version_t types, respectively. Also, some minor "last boy scout rule" changes have been implemented for error handling in the service registration flow.

Error Handling

Most of the string parsing part has been refactored to use open_memstream. Error handling, including error injection testing, is now in place.

codecov-commenter commented 7 months ago

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (95bbcfc) 83.11% compared to head (43b73d3) 84.34%. Report is 29 commits behind head on master.

:exclamation: Current head 43b73d3 differs from pull request most recent head 9d37c55. Consider uploading reports for the commit 9d37c55 to get more accurate results

Files Patch % Lines
libs/framework/src/bundle_context.c 63.63% 8 Missing :warning:
libs/framework/src/dm_component_impl.c 63.63% 8 Missing :warning:
libs/utils/src/filter.c 98.39% 8 Missing :warning:
libs/framework/src/service_registration.c 81.48% 5 Missing :warning:
...te_service_admin_dfi/src/import_registration_dfi.c 0.00% 1 Missing :warning:
libs/framework/include/celix/BundleActivator.h 50.00% 1 Missing :warning:
...ramework/include/celix/dm/ServiceDependency_Impl.h 50.00% 1 Missing :warning:
libs/framework/src/service_registry.c 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #692 +/- ## ========================================== + Coverage 83.11% 84.34% +1.23% ========================================== Files 254 248 -6 Lines 32929 31180 -1749 ========================================== - Hits 27369 26300 -1069 + Misses 5560 4880 -680 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

PengZheng commented 7 months ago

According to OSGi 3.2.7 Filter Syntax 

substring   ::= attr '=' initial any final
initial     ::= () | value
any         ::= '*' star-value
star-value  ::= () | value '*' star-value
final       ::= () | value

value is a string representing the value, or part of one, which will be compared against a value in the filtered properties. If value must contain one of the characters reverse solidus ('\' \u005C), asterisk ('*' \u002A), paren- theses open ('(' \u0028) or parentheses close (')' \u0029), then these characters should be preceded with the reverse solidus ('\' \u005C) character. Spaces are significant in value. Space characters are defined by Character.isWhiteSpace().

If "space characters are significant in value", the following test should pass:

TEST_F(FilterTestSuite, SubStringTest) {
    celix_autoptr(celix_properties_t) props = celix_properties_create();
    celix_properties_set(props, "test", "John Bob Doe");
    celix_properties_set(props, "test2", "*ValueWithStar");
    celix_properties_set(props, "test3", " Value");

    //test filter with mathing subInitial beginning with whitespace
    celix_autoptr(celix_filter_t) filter13 = celix_filter_create("(test3= Value*)");
    EXPECT_TRUE(celix_filter_match(filter13, props));

    //test filter with matching subInitial consisting of spaces
    celix_autoptr(celix_filter_t) filter14 = celix_filter_create("(test3= *)");
    EXPECT_TRUE(celix_filter_match(filter14, props));
}

But filter13 fails and filter14 triggers assertion assert(filter->children && celix_arrayList_size(filter->children) >= 2). I think the beginning white spaces should not be escaped in value, which is quite different from Java properties file format. And a related issue is raised there.

Please have a look at these two issues, @pnoltes

PS: I have not finished my review yet.

pnoltes commented 7 months ago

@PengZheng Thanks for the thorough review and additional commits. I will do the rework based on the left over review comment.s

This will take some time, because I will not have a lot spare time next week.

pnoltes commented 6 months ago

Please have a look at these two issues, @pnoltes .

Thanks, nice catch. I removed the incorrect whitespace removal

pnoltes commented 6 months ago

Review comments have been addressed. Please note that I used a :+1: emoji to indicate my agreement with review comments and to confirm that it has been addressed.