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.

Sign up for GitHub

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

fix: don't release the gil to early #2440

Merged
merged 3 commits into from
Oct 8, 2024
Merged

fix: don't release the gil to early #2440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2c85fc
chore: make sure we catch gil release errors from pybind11
maartenbreddels Oct 7, 2024
1023030
fix: hash object should properly handle null values as 'other'
maartenbreddels Oct 7, 2024
5244572
fix: do not release the gil before returning an object
maartenbreddels Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/vaex-core/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,16 @@ def __str__(self):
]
extra_compile_args.append("-g")
extra_compile_args += extra_dev_options
if os.environ.get("CI"):
# this makes sure we catch error in using pybind11, see https://github.com/vaexio/vaex/issues/2439
extra_compile_args += ["-UNDEBUG"]
else:
extra_compile_args += ["-DNDEBUG"]
if sys.platform == "darwin":
extra_compile_args.append("-mmacosx-version-min=10.9")



# on windows (Conda-forge builds), the dirname is an absolute path
extension_vaexfast = Extension(
"vaex.vaexfast",
Expand Down
225 changes: 126 additions & 99 deletions packages/vaex-core/src/hash_primitives.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,24 +303,26 @@ class hash_base : public hash_common<Derived, T, Hashmap<T, int64_t>> {
py::object key_array() {
py::array_t<key_type> output_array(this->length());
auto output = output_array.template mutable_unchecked<1>();
py::gil_scoped_release gil;
auto offsets = this->offsets();
size_t map_index = 0;
int64_t natural_order = 0;
// TODO: can be parallel due to non-overlapping maps
for (auto &map : this->maps) {
for (auto &el : map) {
key_type key = el.first;
int64_t index = static_cast<Derived &>(*this).key_offset(natural_order++, map_index, el, offsets[map_index]);
output(index) = key;
{
py::gil_scoped_release gil;
auto offsets = this->offsets();
size_t map_index = 0;
int64_t natural_order = 0;
// TODO: can be parallel due to non-overlapping maps
for (auto &map : this->maps) {
for (auto &el : map) {
key_type key = el.first;
int64_t index = static_cast<Derived &>(*this).key_offset(natural_order++, map_index, el, offsets[map_index]);
output(index) = key;
}
map_index += 1;
}
if (this->nan_count) {
output(this->nan_index()) = NaNish<key_type>::value;
}
if (this->null_count) {
output(this->null_index()) = -1;
}
map_index += 1;
}
if (this->nan_count) {
output(this->nan_index()) = NaNish<key_type>::value;
}
if (this->null_count) {
output(this->null_index()) = -1;
}
return output_array;
}
Expand Down Expand Up @@ -383,25 +385,27 @@ class counter : public hash_base<counter<U, Hashmap2>, U, Hashmap2>, public coun
py::object counts() {
py::array_t<value_type> output_array(this->length());
auto output = output_array.template mutable_unchecked<1>();
py::gil_scoped_release gil;
auto offsets = this->offsets();
size_t map_index = 0;
int64_t natural_order = 0;
// TODO: can be parallel due to non-overlapping maps
for (auto &map : this->maps) {
for (auto &el : map) {
// key_type key = el.first;
value_type value = el.second;
int64_t index = key_offset(natural_order++, map_index, el, offsets[map_index]);
output(index) = value;
{
py::gil_scoped_release gil;
auto offsets = this->offsets();
for (auto &map : this->maps) {
for (auto &el : map) {
// key_type key = el.first;
value_type value = el.second;
int64_t index = key_offset(natural_order++, map_index, el, offsets[map_index]);
output(index) = value;
}
map_index += 1;
}
if (this->nan_count) {
output(this->nan_index()) = this->nan_count;
}
if (this->null_count) {
output(this->null_index()) = this->null_count;
}
map_index += 1;
}
if (this->nan_count) {
output(this->nan_index()) = this->nan_count;
}
if (this->null_count) {
output(this->null_index()) = this->null_count;
}
return output_array;
}
Expand Down Expand Up @@ -538,20 +542,22 @@ class ordered_set : public hash_base<ordered_set<T2, Hashmap2>, T2, Hashmap2> {
auto input = values.template unchecked<1>();
auto output = result.template mutable_unchecked<1>();
size_t nmaps = this->maps.size();
py::gil_scoped_release gil;
for (int64_t i = 0; i < size; i++) {
const key_type &value = input(i);
if (custom_isnan(value)) {
output(i) = this->nan_count > 0;
} else {
std::size_t hash = hasher_map_choice()(value);
size_t map_index = (hash % nmaps);
auto search = this->maps[map_index].find(value);
auto end = this->maps[map_index].end();
if (search == end) {
output(i) = false;
{
py::gil_scoped_release gil;
for (int64_t i = 0; i < size; i++) {
const key_type &value = input(i);
if (custom_isnan(value)) {
output(i) = this->nan_count > 0;
} else {
output(i) = true;
std::size_t hash = hasher_map_choice()(value);
size_t map_index = (hash % nmaps);
auto search = this->maps[map_index].find(value);
auto end = this->maps[map_index].end();
if (search == end) {
output(i) = false;
} else {
output(i) = true;
}
}
}
}
Expand All @@ -565,8 +571,11 @@ class ordered_set : public hash_base<ordered_set<T2, Hashmap2>, T2, Hashmap2> {
const key_type &value = values[i];
// the caller is responsible for finding masked values
if (custom_isnan(value)) {
output[i - offset] = this->nan_value;
assert(this->nan_count > 0);
if(this->nan_count > 0) {
output[i - offset] = this->nan_value;
} else {
output[i - offset] = -1;
}
} else {
std::size_t hash = hasher_map_choice()(value);
size_t map_index = (hash % nmaps);
Expand Down Expand Up @@ -630,46 +639,50 @@ class ordered_set : public hash_base<ordered_set<T2, Hashmap2>, T2, Hashmap2> {
if (result.strides()[0] != result.itemsize()) {
throw std::runtime_error("stride not equal to bytesize for output");
}
py::gil_scoped_release gil;
{
py::gil_scoped_release gil;

size_t nmaps = this->maps.size();
auto offsets = this->offsets();
if (nmaps == 1) {
auto &map0 = this->maps[0];
for (int64_t i = 0; i < size; i++) {
const key_type &value = input[i];
// the caller is responsible for finding masked values
if (custom_isnan(value)) {
output[i] = this->nan_value;
// TODO: the test fail here because we pass in NaN for None?
// but of course only in debug mode
assert(this->nan_count > 0);
} else {
auto search = map0.find(value);
if (search == map0.end()) {
output[i] = -1;
size_t nmaps = this->maps.size();
auto offsets = this->offsets();
if (nmaps == 1) {
auto &map0 = this->maps[0];
for (int64_t i = 0; i < size; i++) {
const key_type &value = input[i];
// the caller is responsible for finding masked values
if (custom_isnan(value)) {
if(this->nan_count > 0) {
output[i] = this->nan_value;
} else {
output[i] = -1;
}
} else {
output[i] = search->second;
auto search = map0.find(value);
if (search == map0.end()) {
output[i] = -1;
} else {
output[i] = search->second;
}
}
}
}
} else {
for (int64_t i = 0; i < size; i++) {
const key_type &value = input[i];
// the caller is responsible for finding masked values
if (custom_isnan(value)) {
output[i] = this->nan_value;
// TODO: the test fail here because we pass in NaN for None?
// but of course only in debug mode
assert(this->nan_count > 0);
} else {
std::size_t hash = hasher_map_choice()(value);
size_t map_index = (hash % nmaps);
auto search = this->maps[map_index].find(value);
if (search == this->maps[map_index].end()) {
output[i] = -1;
} else {
for (int64_t i = 0; i < size; i++) {
const key_type &value = input[i];
// the caller is responsible for finding masked values
if (custom_isnan(value)) {
if(this->nan_count > 0) {
output[i] = this->nan_value;
} else {
output[i] = -1;
}
} else {
output[i] = search->second + offsets[map_index];
std::size_t hash = hasher_map_choice()(value);
size_t map_index = (hash % nmaps);
auto search = this->maps[map_index].find(value);
if (search == this->maps[map_index].end()) {
output[i] = -1;
} else {
output[i] = search->second + offsets[map_index];
}
}
}
}
Expand Down Expand Up @@ -790,9 +803,13 @@ class index_hash : public hash_base<index_hash<T2, Hashmap2>, T2, Hashmap2> {
for (int64_t i = 0; i < size; i++) {
const key_type &key = input(i);
if (custom_isnan(key)) {
output(i) = nan_value;
assert(this->nan_count > 0);
if(nan_value == -1) {
if(this->nan_count > 0) {
output(i) = nan_value;
if(nan_value == -1) {
encountered_unknown = true;
}
} else {
output(i) = -1;
encountered_unknown = true;
}
} else {
Expand Down Expand Up @@ -830,15 +847,23 @@ class index_hash : public hash_base<index_hash<T2, Hashmap2>, T2, Hashmap2> {
for (int64_t i = 0; i < size; i++) {
const key_type &key = input(i);
if (custom_isnan(key)) {
output(i) = nan_value;
assert(this->nan_count > 0);
if(nan_value == -1) {
if(this->nan_count > 0) {
output(i) = nan_value;
if(nan_value == -1) {
encountered_unknown = true;
}
} else {
output(i) = -1;
encountered_unknown = true;
}
} else if (input_mask(i) == 1) {
output(i) = null_value;
assert(this->null_count > 0);
if(null_value == -1) {
if(this->null_count > 0) {
output(i) = null_value;
if(null_value == -1) {
encountered_unknown = true;
}
} else {
output(i) = -1;
encountered_unknown = true;
}
} else {
Expand Down Expand Up @@ -936,16 +961,18 @@ class index_hash : public hash_base<index_hash<T2, Hashmap2>, T2, Hashmap2> {
py::array_t<value_type> indices_array(size_output);
auto output = result.template mutable_unchecked<1>();
auto output_indices = indices_array.template mutable_unchecked<1>();
py::gil_scoped_release gil;
// int64_t offset = 0;
size_t index = 0;
{
py::gil_scoped_release gil;
// int64_t offset = 0;
size_t index = 0;

std::copy(indices.begin(), indices.end(), &output_indices(0));
std::copy(indices.begin(), indices.end(), &output_indices(0));

for (auto &el : found) {
std::vector<value_type> &indices = el.second;
for (int64_t i : indices) {
output(index++) = i;
for (auto &el : found) {
std::vector<value_type> &indices = el.second;
for (int64_t i : indices) {
output(index++) = i;
}
}
}
return std::make_tuple(indices_array, result);
Expand Down
Loading
Loading