Skip to content

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

Open
JakeKirk wants to merge 2 commits intoquixdb:masterfrom
JakeKirk:master
Open

Changes to support VS 2017 builds for Win64 bench-marking.#38
JakeKirk wants to merge 2 commits intoquixdb:masterfrom
JakeKirk:master

Conversation

@JakeKirk
Copy link
Copy Markdown

@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
Copy Markdown
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