Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant