CP-49129: Drop global lock around sexpr parsing #5682
Conversation
|
@GabrielBuica you need to sign off the first commit as well |
GabrielBuica
force-pushed
the
private/dbuica/CP-49129
branch
from
13b0488
to
0af3166
Compare
|
There is also master...edwintorok:xen-api:private/edvint/sexpr-safe, which was waiting for these tests. There are a few more ocamlyacc uses, and we should switch those to menhir as well, can you cherry-pick 041f898 |
|
Maybe also modify |
|
I'll move it to draft until I address @edwintorok's comments. |
GabrielBuica
force-pushed
the
private/dbuica/CP-49129
branch
2 times, most recently
from
004d5b0
to
e456fdb
Compare
| @@ -1,6 +1,6 @@ | |||
| (ocamllex db_filter_lex) | |||
|
|
|||
| (ocamlyacc db_filter_parse) | |||
There was a problem hiding this comment.
Are its users wrapping calls with mutexes as well?
There was a problem hiding this comment.
Not directly, so either we had a race condition here on master, or we had another mutex that happened to protect this too.
We should probably backport this PR to Yangtze as well in that case, even though the Sexpression parser was safe due to the mutex, this other usage in the database (part of db_actions.ml) was not, and probably all our calls to get_all_records_where are not thread-safe/subject to race condition and random failures...
The xenopsd cli one doesn't matter, because that is a single-threaded CLI.
|
A xenops test is failing here, is this the same one that you've seen @psafont ? |
|
It'd be useful to keep/add Colin Barr's (@contificate ?) signoff on 46a0708, although the commit message mentions his name, we usually add signoffs if multiple people worked on the same commit (it is fine to keep your signoff there too if you modified it, in this case you moved it into xapi). Did the original commit have no signoff? |
It did have sign-off but can't have been easily cherry picked because the initial repo with the skeleton test is out-of-tree and consists of a single, initial, commit. That said, it is good to know what to do in the case of multiple authorship/committing: |
Initial commit message: "Provides simple code to generate S-expression trees and attempt to parse them across a large number of threads. It serves to demonstrate that ocamlyacc's generated parses are not thread-safe (in that they modify a global variable "env")." Move Colin's unit test for parallel parsing into xapi. Signed-off-by: Colin Barr <colin.barr@cloud.com> Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Make `test_sexpr` run using alcotest. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
From https://ocaml.org/manual/5.1/lexyacc.html on ocamlyacc concurrency sefety: "Parsers generated by ocamlyacc are not thread-safe. Those parsers rely on an internal work state which is shared by all ocamlyacc generated parsers. The menhir parser generator is a better option if you want thread-safe parsers." We currently hold a global lock while calling the sexpr parser, so even if we try to parse a small sexpression, it'll be blocked behind parsing the large one. Switch to Menhir to make this thread-safe. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Using `ocamlyacc` with this change results in a test failure. Thus showing the issue of parallel parsing using `ocamlyacc`. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
We want to be sure there are no more uses of `ocamlyacc`, because of concurrency issues. Menhir should be used instead. Signed-off-by: Gabriel Buica <danutgabriel.buica@cloud.com>
GabrielBuica
force-pushed
the
private/dbuica/CP-49129
branch
from
e456fdb
to
d6d5c4d
Compare
There was a problem hiding this comment.
You're right that git retained authorship information (there is a commit author and committer, and they can be different).
The 'Signed-off-by' is mainly used by the Linux kernel and similar projects, and they recommend adding Signed-off-by as a patch follows its route to mainline, so e.g. people handling the patch can add their own sign-offs: https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed%20off
Add @contificate unit test that demonstrates the limitations of
ocamlyacc's concurrency.Change from
ocamlyacctomenhirDrop the global lock around
sexprparsing to improve performance.