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

Change Travis to build matrix over g++ 8, 9 and 10 #31

Merged
merged 3 commits into from Jul 4, 2020
Merged

Conversation

eddelbuettel added 3 commits Jul 4, 2020
@eddelbuettel eddelbuettel requested a review from knapply Jul 4, 2020
@knapply
knapply approved these changes Jul 4, 2020
Copy link
Collaborator

@knapply knapply left a comment

AWESOME. Thank you!

@eddelbuettel eddelbuettel merged commit 041fac4 into master Jul 4, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@knapply
Copy link
Collaborator

@knapply knapply commented Jul 4, 2020

Did you plan to rebase feature/fparse immediately with this?

NM, got it!

@eddelbuettel
Copy link
Owner Author

@eddelbuettel eddelbuettel commented Jul 4, 2020

Currently looking into that, and because you and I both touched .travis.yml it is less obvious than usually.

@eddelbuettel
Copy link
Owner Author

@eddelbuettel eddelbuettel commented Jul 4, 2020

And adding -Xours does the trick (with the important note in the git rebase help that "ours" and "theirs" are swapped in a rebase. Who ever said git was intuitive? ;-)

@eddelbuettel
Copy link
Owner Author

@eddelbuettel eddelbuettel commented Jul 4, 2020

So if you git reset --hard a few commits and then pull you should be all good again.

@knapply
Copy link
Collaborator

@knapply knapply commented Jul 4, 2020

Ah okay. I needed to grab a fresh copy anyways, so I just re-did that and it's good to go. Thank you!

@eddelbuettel
Copy link
Owner Author

@eddelbuettel eddelbuettel commented Jul 4, 2020

(BTW weird that gcc-7 pukes so reliably. Do we need to worry because "someone somewhere" will have that version?)

@knapply
Copy link
Collaborator

@knapply knapply commented Jul 4, 2020

It's very weird. I'm just assuming someone will encounter it, so I have the fix.

For context, there are two APIs for simdjson: the default one w/ exceptions enabled and the one without. The key difference in usage is how you retrieve JSON element types.

The "no except" API works whether or not exceptions are enabled, so I used that to maximize future flexibility (the data's type/size is already confirmed at this point).

The fix I'm using (that I'll get in there ASAP) is to guard the matrix-builders (the only place it happens) so that the only-for-exceptions-enabled API is used if exceptions are enabled.

Then I just guard the switch that can turn exceptions off on our end so that gcc-7 can only compile w/ exceptions-enabled and thus can only ever touch the only-for-exceptions-enabled API.

The following code reproduces it...

// [[Rcpp::plugins(cpp17)]]
// [[Rcpp::depends(RcppSimdJson)]]

#define SIMDJSON_EXCEPTIONS 0  // 1: simdjson exceptions enabled

#define GUARD_GCC_7 0


#if GUARD_GCC_7 && __GNUC__ && __GNUC__ <= 7
#undef SIMDJSON_EXCEPTIONS // now simdjson.h will set it to 1 and enable exceptions
#define USE_EXCEPTIONS_API 1
#endif


#include <Rcpp.h>
#include <simdjson.h>
#include <simdjson.cpp>

// [[Rcpp::export]]
void test() {
  auto json = R"( [ [1.0,2.0], [3.0,4.0] ] )"_padded;

  simdjson::dom::parser parser;
  // we KNOW this is an array of arrays of doubles
  simdjson::dom::array array = parser.parse(json).get<simdjson::dom::array>().first;

#if SIMDJSON_EXCEPTIONS
  Rcpp::Rcout << "exceptions enabled" << std::endl;
#else
  Rcpp::Rcout << "exceptions disabled" << std::endl;
#endif

#if USE_EXCEPTIONS_API  // can only be used when exceptions are enabled
  Rcpp::Rcout << "exceptions-only API" << std::endl;

  for (simdjson::dom::array sub_array : array) {
    for (auto element : sub_array) {
      auto res = double(element);
    }
  }

#else  // can be used whether or not exceptions are enabled
  Rcpp::Rcout << "exceptions enabled or disabled API" << std::endl;

  for (auto sub_array : array) { // sub_array is still a simdjson::dom::element, compare w/ loop above
    for (auto element : sub_array.get<simdjson::dom::array>().first) {  // segfault
      auto res = element.get<double>().first;                                         
    }
  }

#endif
}

/*** R
test()
*/
  • If you compile w/ g++-7 as is, you'll segfault.
  • If you GUARD_GCC_7, ensuring exceptions are enabled and the exceptions-only API is used, you're fine
    • this is a simplification of what we'll actually be using

This is where it gets even weirder: The segfault seems totally unrelated to R/Rcpp (at least I don't know of how it could be), but the equivalent straight-C++ has no such problem...

#define SIMDJSON_EXCEPTIONS 0

#include "simdjson.h"

int main() {
  auto json = "[[1,2],[3,4]]"_padded;

  simdjson::dom::parser parser;

  simdjson::dom::array array = parser.parse(json).get<simdjson::dom::array>().first;

#if SIMDJSON_EXCEPTIONS
  std::cout << "exceptions enabled" << std::endl;
#else
  std::cout << "exceptions disabled" << std::endl;
#endif

  for (auto sub_array : array) { 
    for (auto element : sub_array.get<simdjson::dom::array>().first) {
      auto res = element.get<double>().first;
      std::cout << res << std::endl;
    }
  }

  return 0;
}

g++-7 -std=c++17 -o test test.cpp simdjson.cpp && ./test

exceptions disabled
1
2
3
4
@eddelbuettel
Copy link
Owner Author

@eddelbuettel eddelbuettel commented Jul 4, 2020

At least you have it under some control. So we can probably add gcc-7 easily enough to the matrix.

@knapply
Copy link
Collaborator

@knapply knapply commented Jul 4, 2020

I'll add it. I'm wrapping up the fix for this, fparse()/fload(), and some housekeeping I've been meaning to get to.

@knapply
Copy link
Collaborator

@knapply knapply commented Jul 5, 2020

Good to go: https://travis-ci.org/github/eddelbuettel/rcppsimdjson/builds/705026024

Any guesses as to what it could be?

@eddelbuettel eddelbuettel deleted the feature/travis branch Jul 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.