Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster param symbol management #6156

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

smuenzel
Copy link
Contributor

@smuenzel smuenzel commented Jul 13, 2022

*::get_param_descrs is called many times during the a run of z3 (in my case, a cegis loop), which causes string_hash to be approximately 5% of the runtime of the program, according to callgrind.
The issues are constant strings that are getting converted to symbols on every call, which requires hashing of the string so that we can check the global symbol table.

I have a proof of concept patch here, which turns a lot of the symbols into static entries in the symbol table.

It's only meant for discussion, not for merging, since afaict, it currently only works in debug mode (since the initialization at startup requires some resources that aren't created yet), and requires the library to eagerly initialize the symbol table. However, it does remove the 5% hashing overhead.

@smuenzel smuenzel force-pushed the param-symbol-management branch from ee3a8ed to a1cb131 Compare July 14, 2022 02:52
@NikolajBjorner
Copy link
Contributor

What is the use case where get_param_descr() gets called repeatedly?
I tried to see if it is internal and didn't spot where this would be.
A fix seems to better avoid symbol alltogether because they are also prone to lock contention (there is a global unique symbol table to intern strings). The fix would change the types used to index params_ref to something else.
Either std::string or a symbol32 (32 byte fixed-length string, enough to hold symbol names).

@smuenzel
Copy link
Contributor Author

I'm also slightly confused by what's happening.
However, I'm changing the timeout of the solver on different runs, and somehow (according to callgrind), my ~600 invocations of the solver lead to 20million string hashes.
The call graph seems to be something like:

tactic2solver::collect_param_descrs
-> binary_tactical::collect_param_descrs
-> unary_tactical::collect_param_descrs
-> simplify_tactic:collect_param_descrs
-> th_rewriter::get_param_descrs
-> *::get_param_descrs for different rewriters

And callgrind says there is a cycle at binary_tactical::collect_param_descrs (maybe just a red herring)

@NikolajBjorner
Copy link
Contributor

NikolajBjorner commented Jul 14, 2022

More likely it is:

param_descrs r;

Every time you set a parameter it collects parameter descriptions to validate the parameter.
A way around is to cache the descriptions.

            if (to_solver(s)->m_descrs.size() == 0) {
                to_solver_ref(s)->collect_param_descrs(to_solver(s)->m_descrs);
                context_params::collect_solver_param_descrs(to_solver(s)->m_descrs);
            }
            params.validate(to_solver(s)->m_descrs);

where in api_solver.h we have added:

struct Z3_solver_ref : public api::object {
    scoped_ptr<solver_factory> m_solver_factory;
    ref<solver>                m_solver;
    params_ref                 m_params;
    symbol                     m_logic;
    scoped_ptr<solver2smt2_pp> m_pp;
    scoped_ptr<cmd_context>    m_cmd_context;
    mutex                      m_mux;
    event_handler*             m_eh;
    param_descrs               m_descrs;

@smuenzel smuenzel force-pushed the param-symbol-management branch from 5317f33 to f2607f1 Compare July 14, 2022 11:02
@smuenzel
Copy link
Contributor Author

I've applied your suggestion, runtime for string_hash has decreased from 5% to 0.32% of my program.
Thanks!

@smuenzel smuenzel marked this pull request as ready for review July 14, 2022 11:39
@NikolajBjorner NikolajBjorner merged commit 2f5fef9 into Z3Prover:master Jul 14, 2022
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.

2 participants