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

Consider turning off construction-location backtraces in Ruby driver #1843

Closed
mlucy opened this issue Jan 11, 2014 · 11 comments
Closed

Consider turning off construction-location backtraces in Ruby driver #1843

mlucy opened this issue Jan 11, 2014 · 11 comments
Assignees
Milestone

Comments

@mlucy
Copy link
Member

mlucy commented Jan 11, 2014

They make it slower than it should be (see more discussion at #1842).

@ghost ghost assigned mlucy Jan 11, 2014
@mlucy
Copy link
Member Author

mlucy commented Jan 11, 2014

There are two options: turn them off by default, or remove them entirely. I think we should probably do the former.

@coffeemug
Copy link
Contributor

Can you describe what this is? Is this something that isn't in the other drivers? I can't read Ruby code :(

@mlucy
Copy link
Member Author

mlucy commented Jan 11, 2014

This is only in the Ruby driver.

Basically, we give two backtraces when a query fails: the first shows where the query was run, and the second shows the line where the erroneous portion of the query was constructed.

Let's say we have this:

0  def get_date_range(tbl, left, right)
1    tbl.between(r.epoch_time(left), r.epoch_time(right), index:'date')
2  end
3
4  def complete_entry_p(entry)
5    entry['field1'] & entry['field2'] & entry['field3'] & entry['field4']
6  end
7
8  get_date_range(r.table('test'), t1, t2).filter{|row| r.not(complete_entry_p(row))}.run

If an error occurs during the between, the construction backtrace shows an error for line 1. If an error occurs during the predicate in the filter, the construction backtrace shows an error for line 5. If an error occurs accessing the table, the construction backtrace shows an error for line 8. In all cases, the execution backtrace shows an error for line 8, because that's where the query is actually run.

(Basically, every time we build a portion of a query we record where it was built, and later if the backtrace we get from the server shows that that portion of the query caused the error, we tell the user where it was built.)

@danielmewes
Copy link
Member

Would be interesting to know how much the performance impact actually is in a benchmark like https://github.com/ajselvig/RubyDatabaseBenchmarks (also see #1798 ).
That benchmark uses pretty simple queries, so there might be applications where it has more impact. Still it would provide us with a rough ballpark figure.

PS:
@nviennot did some tests as part of #1842 . If I read those numbers correctly, the time difference was in the range of 0.1 ms for constructing a query. So I expect relatively little difference in the numbers that we get from the mentioned benchmark, given that we get only ~500 qps in the first place (so it would be a time saving of ~5%). Now in an environment with multiple concurrent clients (or multi-threaded clients), the impact can be significantly larger though.

@nviennot
Copy link
Contributor

This is what I get with rethinkdb next:

Total insert time: 3.6 seconds (555.3 inserts/second)
Total read time: 4.78 seconds (418.5 reads/second)
Total update time: 5.87 seconds (340.5 updates/second)

Without tracking the caller:

Total insert time: 2.95 seconds (678.2 inserts/second)
Total read time: 4.21 seconds (475.0 reads/second)
Total update time: 4.96 seconds (403.3 updates/second)

@danielmewes
Copy link
Member

@nviennot: Cool, thanks for trying that out! Looks like it might well be worth disabling it by default.

@mlucy
Copy link
Member Author

mlucy commented Jan 15, 2014

After some thought, I think we should just get rid of these backtraces. I think they're probably more confusing than helpful for people who aren't me, even though they're cool. Does anyone object strongly?

@mlucy
Copy link
Member Author

mlucy commented Jan 15, 2014

Alright, I think I'm gonna do this.

@danielmewes
Copy link
Member

Yeah it's a really amazing feature, but probably not necessary for most users.

@mlucy
Copy link
Member Author

mlucy commented Jan 15, 2014

This is in review 1129 by @AtnNn .

@mlucy
Copy link
Member Author

mlucy commented Jan 30, 2014

In next.

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

No branches or pull requests

4 participants