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

Fix memory errors when streaming updates with expression columns #1464

Merged
merged 3 commits into from
Jun 29, 2021

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Jun 28, 2021

This PR fixes a memory issue that was encountered on streaming.js and in Python, where trivial constructions of expression columns caused hard-to-debug memory errors in both Javascript and Python. After some debugging and generating a minimal repro in Python, I found the issue in the way we handle capacity and size for the expression tables that live on each context.

Specifically, during each cycle of computing the expressions, the code called set_capacity on each of the expression tables to match the capacity of flattened, and then set_size on each of the tables to match the size of flattened. However, setting the capacity of the t_data_table does not set the capacity of its t_column or t_lstore objects.

I added a call to verify for each expressions table and saw that the transitions table was broken, where the capacity of the column was 8 bytes (the default empty capacity) yet the size was set to 50 bytes (50 rows in the test dataset * 1 byte per uint8_t in the transitions column). Looking at the code of t_lstore::reserve_impl, it seems like the implementation chooses capacity as max(size, capacity) so there isn't an obvious place where the memory corruption happens.

However, replacing set_capacity with reserve - which does change the capacity of the underlying columns and lstore - fixed the issue both in streaming.js and in the Python library. I've added a test in the Python library that fails immediately with various aborts and segfaults on master, and it fully passes consistently on this branch.

Additionally, I added a fix where the underlying t_columns are not allocated when the the table returned from t_data_table::join is created, thus preventing additional unnecessary allocation and reallocation (as the column is immediately replaced by the columns from the two input tables to join).

@sc1f sc1f added bug Concrete, reproducible bugs C++ labels Jun 28, 2021
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for the PR!

@texodus texodus merged commit a886d57 into master Jun 29, 2021
@texodus texodus deleted the fix-exprtk branch June 29, 2021 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants