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

Changes to support VS 2017 builds for Win64 bench-marking. #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JakeKirk
Copy link

@JakeKirk JakeKirk commented Aug 21, 2018

This is my first pull request, bear with me as I learn how this all works.

Here are the change notes:
Changes have been tested on Kubuntu gcc/cc 7.3.0 and VS 2017 (Win64).

  • Added cross platform "getopts" parg.c/h to assist with this goal.
  • Reworked benchmark_codec_with_options()
    -#ifdef conditionalize fork(), exit() and wait() logic.
    • added FILENAME_MAX for fifo_name size, whether mktemp() or GetTempFileNameA() is used.
  • Created append_csv_benchmark() to assist with readabilty and simplify Win32/Linux changes.
  • mktemp() on Win32 creates only 26 unique temp files names (which normally are deleted if no errors occur, but can fail if file pre-exists.
    Therefore decided to use GetTempFileNameA() instead which creates a larger set of temporary file names.
  • Added a few comments, made a few indentation changes (hope tabs match!), added a few constants.

Changes have been tested on Kubuntu and Win32.
- Added cross platform "getopts" parg.c/h to assist with this goal.
- Reworked benchmark_codec_with_options()
   -#ifdef conditionalize fork(), exit() and wait() logic.
   - added FILENAME_MAX for fifo_name size, whether mktemp() or GetTempFileNameA() is used.
- Created append_csv_benchmark() to assist with readabilty and simplify Win32/Linux changes.
- mktemp() on Win32 creates only 26 unique temp files names (which normally are deleted if no errors occur, but can fail if file pre-exists.
Therefore decided to use GetTempFileNameA() instead which creates a larger set of temporary file names.
- Added a few comments, made a few indentation changes (hope tabs match!), added a few constants.
The fifo_name results file must be opened using O_BINARY otherwise the CSV data will be misaligned.
@JakeKirk
Copy link
Author

JakeKirk commented Aug 23, 2018

On Win32...
By default open() without the O_BINARY flag appears to be in "text mode" and if the results intermittently contain a Ctrl-Z value... the read() then fails/stops prematurely (and the results where not written/moved to CSV and were missing, but logs show that the "level" had been completed). So this fixes the intermittent missing "level" results in the CSV. The bytes_read didn't match sizeof(SquashBenchmarkResult), bytes_read was something less and processing was skipped. Error logging of failure to read less than the expected bytes was also added. I also thought about #pragma pack(1) alignment on SquashBenchmarkResult but it doesn't seem to be needed if the O_BINARY flag is used. This also fixes a bad data alignment side-effect. What I saw in the CSV was a structure alignment read/write issue and looked like this (negative #'s, missing data, extremely large values):
MyDataSet1.DZT,brotli,brotli,3,2052096,218762561,-0.000000,-0.000000,-0.000000,-0.000000
MyDataSet1.DZT,brotli,brotli,10,2052096,168680826,-82280922714874703238086033564862959098824282080290866632698245024619752864926095607914748551723551

@JakeKirk JakeKirk closed this Aug 23, 2018
@JakeKirk JakeKirk reopened this Aug 23, 2018
@JakeKirk JakeKirk changed the title Changes to support VS 2017 builds for Win32 benchmarking. Changes to support VS 2017 builds for Win64 bench-marking. Aug 23, 2018
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.

1 participant