FW/logging: remove use of FILE* the child logging #286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
That is instead wrapping a file descriptor, instead of a FILE*. This is the last use of FILE* for logging. Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
We can do everything with low-level file descriptors now. Clean-ups in upcoming commits. Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
It didn't use to be there because we kept it together with the FILE* stream from <stdio.h>, but since we don't use that any more, we can move. This also removes the need to keep it in the ExecState that is passed to the child process in execve() mode. Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
Avoids having to repeat ourselves and will allow some further refactoring to come. The first of which will be to rename those variables to more sensible and less similar names. Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
They were too similarly named. Now it's clear one is referring to the child process (soon to be children processes) exit status and the other is the full test's result. Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
fenrus75
approved these changes
Jun 30, 2023
thiagomacieira
added a commit
to thiagomacieira/opendcdiag
that referenced
this pull request
Aug 31, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (opendcdiag#286) removed the `FILE*`
-based logging and moreover 8294f1a
(opendcdiag#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
thiagomacieira
added a commit
to thiagomacieira/opendcdiag
that referenced
this pull request
Aug 31, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (opendcdiag#286) removed the `FILE*`
-based logging and moreover 8294f1a
(opendcdiag#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
thiagomacieira
added a commit
to thiagomacieira/opendcdiag
that referenced
this pull request
Aug 31, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (opendcdiag#286) removed the `FILE*`
-based logging and moreover 8294f1a
(opendcdiag#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
thiagomacieira
added a commit
to thiagomacieira/opendcdiag
that referenced
this pull request
Sep 1, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (opendcdiag#286) removed the `FILE*`
-based logging and moreover 8294f1a
(opendcdiag#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
thiagomacieira
added a commit
to thiagomacieira/opendcdiag
that referenced
this pull request
Sep 1, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (opendcdiag#286) removed the `FILE*`
-based logging and moreover 8294f1a
(opendcdiag#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
thiagomacieira
added a commit
that referenced
this pull request
Sep 5, 2023
This is where it used to be, in the earliest versions of the framework.
I moved it to the child on d0edbae7ad0dde41a3a1b2a41ebfc957f16b409f
(2021-03-19) in the old repository:
FW: move the per-test check on CPU features and timing into the child
This means we'll launch a child for tests that will be skipped, thus
adding a bit of overhead. But it simplifies logging (especially for
Windows support).
Before this, we were getting away with the `FILE *` logging across
parent and child on Linux, but that was a ticking time bomb. Therefore,
I simplified by moving all the per-thread logging into the child.
Commit 040bc36 (#286) removed the `FILE*`
-based logging and moreover 8294f1a
(#324) kept the file descriptors open, so we can now easily bring it
back up to the parent. This saves us from launching the child processes
when we know we're going to skip -- this is important on Windows where
starting processes is relatively expensive.
Signed-off-by: Thiago Macieira <thiago.macieira@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We already opened the
FILE *in unbuffered mode, so the stdio content was only overhead we didn't need. The few places where we still used it were removed in PR #278. By using only low-level file descriptors, we can store the file descriptor directly in the shared memory segment, reducing the size of thatExecStatethat is passed to the child process (and also removing one more hardcodedMAX_THREADS).The other commits are clean-ups.