Issue 6574 - Move pblock access from switch to func table #6575

mreynolds389 · 2025-02-03T20:10:23Z

Description

Currently there are over 300 switch cases for getting/setting pblock parameters.

Instead we should have separate functions for each parameter, and use a callback function pointer table.

This also combines all the parameter definitions to a single ordered list. This will make future changes much easier to implement.

Relates: #6574

mreynolds389 · 2025-02-03T21:04:51Z

As for performance, with ldclt doing exact searches I do see a 5% - 8% performance improvement when the number of threads is increased to 16 (matching the hardware). Under less load I don't really see an improvement. So there is a benefit when the server is being hit hard.

Firstyear

Not every day you get a 5 to 8% improvement! That's amazing to see :)

mreynolds389 · 2025-02-03T23:40:46Z

Not every day you get a 5 to 8% improvement! That's amazing to see :)

Well its only under certain load types - it's not across the board :-) But yes I see an improvement when all the workers are working

droideck

Besides a couple of minor issues, it looks like a simple but powerful fix! Nice find!

droideck · 2025-02-04T03:11:13Z

ldap/servers/slapd/pblock.c

-#ifdef PBLOCK_ANALYTICS
-    pblock_analytics_record(pblock, arg);
-#endif


This analytics code (and the one in _set) were removed. Should we still keep it? Or it's not used?

This analytics code (and the one in _set) were removed. Should we still keep it? Or it's not used?

I'm not 100% what these are for. I assumed it was for measuring how we accessed the pblock, but I have a feeling this is no longer needed. Perhaps we can remove it in a separate PR?

droideck · 2025-02-04T03:34:08Z

ldap/servers/slapd/slapi-plugin-compat4.h

-/******* Deprecated (obsolete) pblock arg identifiers ************************/
-#define SLAPI_CONFIG_FILENAME 40 /* use config. entries instead */
-#define SLAPI_CONFIG_LINENO 41   /* use config. entries instead */
-#define SLAPI_CONFIG_ARGC 42     /* use config. entries instead */
-#define SLAPI_CONFIG_ARGV 43     /* use config. entries instead */
-#define SLAPI_CONN_AUTHTYPE 144  /* use SLAPI_CONN_AUTHTYPE */
-#define SLAPI_REQUESTOR_ISUPDATEDN SLAPI_IS_REPLICATED_OPERATION
-#define SLAPI_CONN_CLIENTIP 145 /* use SLAPI_CONN_CLIENTNETADDR */
-#define SLAPI_CONN_SERVERIP 146 /* use SLAPI_CONN_SERVERNETADDR */
-


I think we should mention the removal in the commit message.

But also, we have some leftovers in test/libslapd/pblock/pblock_accessors* and a bit in _slapi_pblock_deprecated in ldap/servers/slapd/pblock_v3.h.

IMO, it'll be cleaner to remove the code in a separate PR.

I left this as is with the new commit

tbordaz

The change looks good and perf improvement very nice. IIUC a former customer plugin will require to be rebuilt, correct ?

progier389 · 2025-02-04T10:55:42Z

ldap/servers/slapd/slapi-plugin.h

@@ -6977,50 +6974,6 @@ slapi_timer_result slapi_timespec_expire_check(struct timespec *expire);
                                         * into tombstone
                                         */

-/*


I do not like the massive change in slapi-plugin.h
(looks like a risk of breaking existing custom plugin
And after a closer look as you changed the #define values, it will definitively break the binary compatibility
with existing custom plugin.
IMHO we should not do this. we should be able to split the switches without changing slapi-plugin.h

an option would be to have a conversion table that convert a former define value into an index into the callback table

FYI, or you can do a simple table:
here is a python script that generate a skeleton of what it would looks like:
split_pblock_switches.py.txt

mreynolds389 · 2025-02-04T12:46:51Z

The change looks good and perf improvement very nice. IIUC a former customer plugin will require to be rebuilt, correct ?

@progier389 @tbordaz Yeah you guys are right, but the problem is that the callback table will have thousands of empty slots to fill in the gaps between the #define numbers. Maybe we can use a hash table instead?

progier389 · 2025-02-04T13:40:53Z

Having a single table with hole will be faster than an hash table (there will be a single indirection versus hash computation)
In fact if we ignore SLAPI_HINT (which is purely internal and can be changed) the maximum value is slightly below 2000
Having hole is not really an issue: holes are just a pointeur to a function that logs error

mreynolds389 · 2025-02-04T14:02:44Z

Having a single table with hole will be faster than an hash table (there will be a single indirection versus hash computation) In fact if we ignore SLAPI_HINT (which is purely internal and can be changed) the maximum value is slightly below 2000 Having hole is not really an issue: holes are just a pointeur to a function that logs error

I guess I got caught up in maintainability and trying to fix how sloppy our current code is (in regards to defines). Anyway I'll undo the slapi-plugin change and adjust the tables in pblock.c

droideck · 2025-02-04T20:33:29Z

ldap/servers/slapd/pblock.c

+};
+
+int
+slapi_pblock_set(Slapi_PBlock *pblock, int arg, void *value)


I think we still need PR_ASSERT(NULL != pblock); here.

I think we still need PR_ASSERT(NULL != pblock); here.

Agreed, done!

Description Currently there are over 300 switch cases for getting/setting pblock parameters. Instead we should have separate functions for each parameter, and use a callback function pointer table. Relates: 389ds#6574 Reviewed by: spichugi, firstyear, progier, tbordaz (Thanks!!!!)

Firstyear approved these changes Feb 3, 2025

View reviewed changes

droideck approved these changes Feb 4, 2025

View reviewed changes

tbordaz reviewed Feb 4, 2025

View reviewed changes

progier389 reviewed Feb 4, 2025

View reviewed changes

mreynolds389 force-pushed the pblock_reform branch from 005db05 to 1b70acc Compare February 4, 2025 18:40

mreynolds389 requested review from progier389 and tbordaz February 4, 2025 18:42

droideck reviewed Feb 4, 2025

View reviewed changes

mreynolds389 force-pushed the pblock_reform branch from 1b70acc to b51a417 Compare February 4, 2025 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 6574 - Move pblock access from switch to func table #6575

Issue 6574 - Move pblock access from switch to func table #6575

mreynolds389 commented Feb 3, 2025

mreynolds389 commented Feb 3, 2025

Firstyear left a comment

mreynolds389 commented Feb 3, 2025

droideck left a comment

droideck Feb 4, 2025

mreynolds389 Feb 4, 2025

droideck Feb 4, 2025

mreynolds389 Feb 4, 2025

tbordaz left a comment

progier389 Feb 4, 2025

tbordaz Feb 4, 2025

progier389 Feb 4, 2025

mreynolds389 commented Feb 4, 2025

progier389 commented Feb 4, 2025

mreynolds389 commented Feb 4, 2025 •

edited

Loading

droideck Feb 4, 2025

mreynolds389 Feb 4, 2025

Issue 6574 - Move pblock access from switch to func table #6575

Are you sure you want to change the base?

Issue 6574 - Move pblock access from switch to func table #6575

Conversation

mreynolds389 commented Feb 3, 2025

mreynolds389 commented Feb 3, 2025

Firstyear left a comment

Choose a reason for hiding this comment

mreynolds389 commented Feb 3, 2025

droideck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbordaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreynolds389 commented Feb 4, 2025

progier389 commented Feb 4, 2025

mreynolds389 commented Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mreynolds389 commented Feb 4, 2025 •

edited

Loading