-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add support for stitcher configuration #321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of introducing a config struct, on the presumption that we will need to add more configuration knobs in the future. Just a couple of comments on the details:
stack-graphs/src/c.rs
Outdated
@@ -1471,6 +1495,7 @@ pub extern "C" fn sg_forward_partial_path_stitcher_from_partial_paths( | |||
partials: *mut sg_partial_path_arena, | |||
count: usize, | |||
initial_partial_paths: *const sg_partial_path, | |||
config: sg_stitcher_config, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but maybe counter-intuitively, here I think you should pass in the config by pointer! (We're not currently worrying about versioning our C ABIs, but if we were to do that, passing by pointer means that the ABI calling convention for this function would not change even if we were to add more fields to the config struct.)
To clarify, would this supersede |
Yes, I think so.
I've been wondering this too. They feel different to me. The similar path detection is required, perhaps not for correctness, at least for feasibility (for certain rule sets). The max work is more an operational thing, more like a cancellation flag even? The similar path setting is something that should be part of a language configuration, I think, while the work per phase certainly not. My idea was that the language configuration would just contain a stitcher configuration value, but perhaps that is not the right approach. It really depends on what other settings we envision? One I could see happening is whether the stitcher tracks statistics or not, but this would also be an operational thing, more like the max work, than this. |
I hear that, though for me, the "intent" of each knob is less relevant than the fact that there is a knob. So we should standardize on either one of the following, but not a mixture of both:
The benefit of (1) is that we don't have to finagle a Go-style 0 value that is "a reasonable default" for each knob — the default is in place if you don't call the setter function. The benefit of (2) is that there is less of a proliferation of functions.
This is the part that would suggest a change from (1) to (2). But I think I'd still have a slight preference for (1). You could still have a stitcher config "section" in the language configuration types — i.e., as a separate embedded type, like we're doing with |
I think you're right. Trying to shoehorn these two use cases into one type doesn't work. Re (1) and (2), I'll see how that works out for passing things into more high-level functions like the test runner. But perhaps the values from the language configuration will be all we need for that, and the others will always be chosen by the implementation. |
9069c45
to
064155e
Compare
4921589
to
a92b9b0
Compare
Performance SummaryComparing base d7bb4ad with head a92b9b0 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
Performance SummaryComparing base d7bb4ad with head ad9c0da on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
The changes ended up like this:
|
1ffe3cf
to
e46feb3
Compare
Performance SummaryComparing base 3696992 with head e46feb3 on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
Performance SummaryComparing base 3696992 with head 405a28b on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
405a28b
to
294596a
Compare
Performance SummaryComparing base 5a6744b with head 294596a on typescript_benchmark benchmark. For details see workflow artifacts. Note that performance is tested on the last commits with changes in Before
After
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit, otherwise 👍
@@ -36,5 +36,6 @@ harness = false # need to provide own main function to handle running tests | |||
[dependencies] | |||
anyhow = "1.0" | |||
clap = { version = "4", features = ["derive"] } | |||
stack-graphs = { version = "0.12", path = "../../stack-graphs" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this spurious? I don't see a change in the crate's Rust code that needs to refer to stack-graphs
directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|
||
impl Into<StitcherConfig> for sg_stitcher_config { | ||
fn into(self) -> StitcherConfig { | ||
StitcherConfig::default().with_detect_similar_paths(self.detect_similar_paths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is nice, it means we don't have to #[repr(C)]
the Rust type
« #346 | #326 »
The static methods on
ForwardPartialPathStitcher
do not allow configuration of stitcher properties. This PR adds aStitcherConfig
type to remedy that.