Skip to content

Conversation

jeremyevans
Copy link
Contributor

If caller/caller_locations is passed a block, the block is yielded
the string or Thread::Backtrace::Location object as the VM frames
are being scanned. The caller can break/return for an early exit.
This allows for constructing partial backtraces where you don't
know the size or level of the backtrace up front, as the part
of the backtrace you want depends on the location.

This has rb_ec_partial_backtrace_object accept to_str and do_yield
arguments. If do_yield is true, then the array of backtrace
strings/locations is created up front, and for each frame of the
backtrace, after the string/location is added to the array, it is
yielded to the block. If the array is created up front, we skip
recreating it after the scan.

Implements [Feature #16663]

Documentation/NEWS not modified yet, I will take care of that
before merging if this is accepted.

If caller/caller_locations is passed a block, the block is yielded
the string or Thread::Backtrace::Location object as the VM frames
are being scanned.  The caller can break/return for an early exit.
This allows for constructing partial backtraces where you don't
know the size or level of the backtrace up front, as the part
of the backtrace you want depends on the location.

This has rb_ec_partial_backtrace_object accept to_str and do_yield
arguments.  If do_yield is true, then the array of backtrace
strings/locations is created up front, and for each frame of the
backtrace, after the string/location is added to the array, it is
yielded to the block.  If the array is created up front, we skip
recreating it after the scan.

Implements [Feature #16663]
@jeremyevans jeremyevans requested review from headius and ko1 October 26, 2021 19:28
@ko1
Copy link
Contributor

ko1 commented Nov 8, 2021

I believe @jeremyevans' code but for the spec it should get Matz's approval.

@jeremyevans
Copy link
Contributor Author

I believe @jeremyevans' code but for the spec it should get Matz's approval.

Correct. I added this to the next dev meeting earlier today.

@eregon
Copy link
Member

eregon commented Nov 8, 2021

This has rb_ec_partial_backtrace_object accept to_str and do_yield
arguments. If do_yield is true, then the array of backtrace
strings/locations is created up front, and for each frame of the
backtrace, after the string/location is added to the array, it is
yielded to the block. If the array is created up front, we skip
recreating it after the scan.

Does it mean all entries are created upfront? If so I think that defeats the point of this feature, which should only walk the necessary frames (which is also what can make it tricky to implement).

@jeremyevans
Copy link
Contributor Author

This has rb_ec_partial_backtrace_object accept to_str and do_yield
arguments. If do_yield is true, then the array of backtrace
strings/locations is created up front, and for each frame of the
backtrace, after the string/location is added to the array, it is
yielded to the block. If the array is created up front, we skip
recreating it after the scan.

Does it mean all entries are created upfront? If so I think that defeats the point of this feature, which should only walk the necessary frames (which is also what can make it tricky to implement).

No. This yields to the block as each entry is created, it does not create all entries up front. The comment states that if an array was created up front (only done if a block is passed), then we don't create a new array for the backtrace if we get to the end of the array, we just use the already created array.

@eregon
Copy link
Member

eregon commented Nov 9, 2021

Ah I see, thanks for the clarification.
I think implicitly adding the entries to the array is an extra cost (notably it's likely to force the allocation and prevent escape analysis), if the method takes a block, then I think it should return nil and the block can decide what to do with the backtrace entries.

@ko1
Copy link
Contributor

ko1 commented Dec 10, 2021

Now caller with block proposal is rejected. Can we close this PR?

@jeremyevans
Copy link
Contributor Author

Sure.

@headius
Copy link
Contributor

headius commented Dec 11, 2021

I missed the ping for this until now. Unfortunate that it was rejected. Consumers of caller frequently do not specify a stack trace size and just mine the resulting full trace array for only a few frames. Rather than forcing them to guess how many frames they really need, this proposal would have allowed them to consume as many as they needed without paying a cost for the frames they do not need.

@headius
Copy link
Contributor

headius commented Dec 11, 2021

I replied to the ruby-lang issue here: https://bugs.ruby-lang.org/issues/16663#change-95288

@jeremyevans
Copy link
Contributor Author

@headius The particular approach in this pull request was rejected, however, matz is still open to the feature. Once we get consensus on how the feature should work, I'll submit another PR to implement it.

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.

4 participants