perf: do not search and dlopen on every sql cmd #452
Conversation
| _instances = {} | ||
| _lock = threading.Lock() # Ensures thread safety | ||
|
|
||
| def __call__(cls, *args, **kwargs): |
There was a problem hiding this comment.
I remain undecided. While I appreciate your insightful observation regarding the contentious nature of metaclasses, in this specific instance—where the singleton pattern is widely recognized—it may be acceptable.
Ultimately, the choice of which singleton implementation (each has its own drawbacks) to adopt is largely a matter of personal preference and not probabilistic exercise. Therefore, I would value the perspective of a real human being on this matter.
isimluk
force-pushed
the
perf-sql-dlopen
branch
from
44456c8 to
1710540
Compare
This extracts a handle of zen-internal FFI to a thread safe singleton.
Ensuring the DLL is searched for and loaded only once. Previously, we
would run multiple syscalls to get the location of the dll/so and then
built the python datastructures (CDLL object) that has to be later GCed.
I haven't microbenchmarked the improvements rigorously, as I feel GC
savings may contribute considerably in real-world scenario.
Although,
poetry run pytest -rP ./aikido_zen/vulnerabilities/sql_injection/
improves by over 4.3% on my set-up.
to its natural habitat
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| _instances = {} | ||
| _lock = threading.Lock() # Ensures thread safety | ||
|
|
||
| def __call__(cls, *args, **kwargs): |
There was a problem hiding this comment.
The call method implements complex singleton pattern with thread safety using atomic check-and-create operations, but lacks explanatory comments to help developers understand the locking mechanism and why it's necessary for thread-safe singleton creation. More info
|
|
||
| class ZenInternal(metaclass=__Singleton): | ||
| # Reference : [rust lib]/src/sql_injection/helpers/select_dialect_based_on_enum.rs | ||
| SQL_DIALECTS = { |
There was a problem hiding this comment.
we have helper function for this btw
There was a problem hiding this comment.
Do you mean map_dialect_to_rust_int?
I took that down ✂️ in the second commit.
I felt that this fn carried intimate knowledge of the FFI/zen-internal and thus should be part of the class that deals with these matters. Further, I decided to expand the method and leave only the map in there as I saw that these things are contextually well collocated and there is no need for it to exist as separate fn.
There was a problem hiding this comment.
True, generally we just don't like to refactor stuff & the helpers can include logic specific to parts of code, it's just a way of abstracting away more tedious stuff that rarely changes (like this map). For me it's fine either way, but it's easier to leave it as is and cut down on the refactors (at least that's my opinion).
perf: do not search and dlopen on every sql cmd
This extracts the handler of zen-internal FFI to a thread safe singleton.
Ensuring the DLL is searched for and loaded only once. Previously, we
would run multiple syscalls to get the location of the dll/so and then
built the python datastructures (CDLL object) that has to be later GCed.
I haven't microbenchmarked the improvements rigorously, as I feel GC
savings may contribute considerably in real-world scenario.
Although,
improves by over 4.3% on my set-up.