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

Deps/opmsg #2306

Closed
wants to merge 6 commits into from
Closed

Deps/opmsg #2306

wants to merge 6 commits into from

Conversation

fewf
Copy link
Contributor

@fewf fewf commented May 7, 2019

Hi @ry -- this is following up on #2223

Definitely not finished, but you can let me know if it's going in the right direction. I hacked modules into the ThreadSafeState object, so it can compile, but it's not behaving--I'm sure for obvious reasons. I'll leave some more comments around the code. Also thinking we'll need to add in some tests somewhere/

table Deps {}

table DepsRes {
data: [ubyte];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't sure if this should be type string or not, since it could be arbitrarily large

}
let out = maybe_out.unwrap();

if let Some(deps) = state.modules.deps(&out.module_name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this does not work right now

js/deps.ts Outdated
import { assert } from "./util";
import * as flatbuffers from "./flatbuffers";

export function deps(): any {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

def want to come back and drop this any return type/create Interface for deps, etc


let maybe_out = worker::fetch_module_meta_data_and_maybe_compile(
&state,
&state.main_module().unwrap(),
Copy link
Member

@bartlomieju bartlomieju May 7, 2019

Choose a reason for hiding this comment

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

I think you can easily take module name from message to allow to inspect any file:

Deno.deps("https://deno.land/log/mod.ts");

EDIT: One thing to keep in mind then is that fetch_module_meta_data_maybe_compile is used during boot, so it will print "Downloading" and "Compiling" messages

Copy link
Member

Choose a reason for hiding this comment

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

I think you can easily take module name from message to allow to inspect any file:

I'm thinking about it, but it's not obvious to me how to find out which module as called an op... It seems this is some functionality we need in core.

@bartlomieju bartlomieju mentioned this pull request May 9, 2019
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

@fewf Sorry for the slow response! Good start on this - it's brought up a few questions

  1. Using the main module as the referrer isn't correct - we need to somehow get the information about which module is calling an op. This is not obvious to me how to do this, but clearly would be very useful. I will research.
  2. We need to move the modules from Worker into ThreadSafeState.

@@ -65,6 +66,7 @@ pub struct State {
pub start_time: Instant,
pub resource: resources::Resource,
pub dispatch_selector: ops::OpSelector,
pub modules: deno::Modules,
Copy link
Member

Choose a reason for hiding this comment

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

This is something we need to do. However it needs to also be removed from the Worker struct. I will prepare a PR which does just this.

Copy link
Member

Choose a reason for hiding this comment

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

Modules had been moved to State - @fewf you can merge latest changes from master

@ry
Copy link
Member

ry commented May 9, 2019

After some research, it doesn't appear V8 gives us much in the way of determining which module called a function.

One way this might be accomplished is by manually traversing the current stack to figure out
It seems this could be determined is by calling v8::StackTrace::CurrentStackTrace(). It's not something we could do for every op, for performance reasons, but maybe it could be called during deps() or other ops that need it. Maybe something like const char* deno_calling_module(Deno* d) ...

@fewf
Copy link
Contributor Author

fewf commented May 10, 2019

Thanks for the info @ry. I think I understand the issue--it's that if A has B for a dependency and B calls deps, it should return only B's dependencies, right? That does seem tricky. I notice console.trace is not implemented in Deno--is that a todo? There's a way to get a stack trace like so (cribbed from https://stackoverflow.com/a/28118170 ):

  let stack;

  try {
    throw new Error('');
  }
  catch (error) {
    stack = error.stack || '';
  }

And that seems to work in Deno.

For the moment, I've pushed up some more proposed changes to better Typescript-ify the deps function and add the option to return an object instead of an array mentioned here #2096 (altho maybe that functionality should be elsewhere). I tried to rely on conventions I could find elsewhere.

@ry
Copy link
Member

ry commented Jul 23, 2019

@fewf I'm closing stale PRs and I'm afraid this one qualifies. We've done a lot of refactoring in modules recently and I imagine this would need some strenuous rebasing. I'm going to close this - feel free to hit me up on gitter and discuss if you want to pursue. Sorry for not getting it in sooner - but there were some open questions.

@ry ry closed this Jul 23, 2019
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.

3 participants