-
Notifications
You must be signed in to change notification settings - Fork 36
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
this code very faster #539
base: master
Are you sure you want to change the base?
Conversation
TODO list:
|
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.
Looking good so far. I'm gonna try review frequently to keep the diff size small
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.
Nice, continues to get closer :)
…ermediate representation
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.
Nice, this is mcuh easier to understand now
Some preliminary performance metrics. (Take with a grain of salt, as with all benchmarks.)
This fork:
Current Master (4.2.27) after running a few times to get JIT advantages:
This fork is about |
Some fixes after a few tests
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.
Maybe I'm missing something, but it seems alot of the references explicit
need to be inverted
const traversal_options = new TraversalOptions( | ||
[file_path], | ||
options.fields ?? [], |
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.
Question here. Previously the fallback was []
. Does your traversal treat an empty fields list as all fields allowed? Cause mine needed you to explicitly pass in the full list of fields, if that's what you wanted
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.
undefined
is all fields, []
is none
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.
Ok call, I'll update my change to use undefined
when necessary
@@ -79,21 +80,20 @@ impl EdgeData { | |||
// the mapping that exist on the JS side are as follows | |||
// "field" | "explicit" | "source" | "implied_kind" | "round" | |||
|
|||
// TODO: maybe change the attribute options so that the JS side better matches the data | |||
|
|||
// TODO(JS): maybe change the attribute options so that the JS side better matches the data |
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 could've named it better, but with the TODO(RUST)
comments, I more just meant TODOs left for this PR, not where the todo needs to be done :)
if self.stopped() { | ||
LOGGER.warn(&format!("PerfLogger {} is already stopped", self.name)); | ||
} else { |
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.
If you double-check that it's not already stopped in this function, it feels unecessary to check this before calling self.stop()
elsewhere
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.
Kind of, the check here is to make sure that the place that is calling this checked stopped before.
Yep, it seems i was quite tired when I made those last few commits and missed a lot of places 😅 |
|
||
export const update = () => { | ||
const max_depth = options.depth[1] ?? DEFAULT_MAX_DEPTH; | ||
|
||
const source_path = options["start-note"] || file_path || $active_file_store?.path || ""; |
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.
file_path
is start-note
, if present
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 ok
wasm/src/edge_sorting.rs
Outdated
pub fn sort_edges(&self, graph: &NoteGraph, edges: &mut [EdgeStruct]) { | ||
let ordering = self.get_edge_ordering(graph); | ||
|
||
edges.sort_by(|a, b| self.apply_edge_ordering(&ordering, a, b)); | ||
edges.sort_by(|a, b| self.apply_edge_ordering(ordering.as_ref(), a, b)); | ||
} |
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.
Are these clippy suggestions? What do they mean?
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 can explain this at some point in VC.
Co-authored-by: Ross <[email protected]>
Co-authored-by: Ross <[email protected]>
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.
Nice, I like the use of getter_with_clone
:)
pub fn to_paths(&self) -> Vec<Path> { | ||
self.paths.clone() | ||
} |
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.
Isn't this just a getter on self.paths
|
||
#[wasm_bindgen] | ||
#[derive(Clone, Debug)] | ||
pub struct FlatRecTraversalResult { |
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 at a glance, can we use this same struct to handle the flatten
method on RecTraversalData? It seems there's some duplicated logic
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.
it's similar, but there are some differences, though the names are too close for my liking. Maybe they can be put into one method though,
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.
Nice, looking good
🦀
(feel free to change the name of this PR)