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

Eval elimination #6 #5934

Closed
wants to merge 2 commits into from
Closed

Eval elimination #6 #5934

wants to merge 2 commits into from

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Dec 14, 2017

Eliminate new Function use by overriding toString on function (and displayName property for Firefox), which makes it appear as named function in debugger regardless of original name or lack of it, also there is no call-time overhead.

Haven't tested other browsers, as I'm on Linux and never use anything besides Firefox and Chromium for debugging anyway.

This is also the first step for NO_DYNAMIC_EXECUTION elimination from #5911

Can be tested in any browser with following:

function my_func () {}
console.log(my_func)
console.log(function () {})
function make_my_func (func) {
	func.displayName = 'my_func';
	func.toString = function () {return 'function my_func () {}'};
	return func;
}
console.log(make_my_func(function () {}))

…d `displayName` property for Firefox), which makes it appear as named function in debugger regardless of original name or lack of it, also there is no call-time overhead
…s, now they are more independent, some embinder tweaks to make this possible
@nazar-pc
Copy link
Contributor Author

I have more stuff on the topic, but let's land this part first

@kripken
Copy link
Member

kripken commented Dec 15, 2017

@AlexPerrot, can you please look at this?

@AlexPerrot
Copy link
Contributor

@kripken Sorry, my new job doesn't leave much time to work on embind.
At first glance, it seems ok, but hopefully I should be able to look at it more thoroughly before 2018.

@kripken
Copy link
Member

kripken commented Mar 1, 2018

ping @AlexPerrot, any chance you have time for this?

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Mar 1, 2018

As discussed in referenced issue, createNamedFunction is only used for debugging and the changes from this PR do not make debuggers happy to print proper function names in stacktrace.
So we should either drop it entirely or leave this new Function with the only purpose been debugging.

I'd bet on the latter, but either way the decision should be made and PR shouldn't be merged as is because createNamedFunction is mostly useless.

@kripken
Copy link
Member

kripken commented Mar 5, 2018

I see, thanks @nazar-pc . Ok, let's close this, and we'll tolerate having some new Function in debugging code.

@kripken kripken closed this Mar 5, 2018
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Mar 6, 2018

@kripken, is there any other flag like DEBUG that we can replace NO_DYNAMIC_EXECUTION with in this instance? I think the code branch with new Function should only be used for debugging purposes and by default an alternative branch should be used.

@kripken
Copy link
Member

kripken commented Mar 6, 2018

I'm not very familiar with the embind code. What is the specific meaning of debug here? I mean, how would you debug this code - would it be something you'd want by default in a debug build (like an assert), or something you would set a flag specifically for (like LIBRARY_DEBUG or GL_DEBUG maybe)?

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Mar 6, 2018

I'm not familiar with embind code specifically either, but the idea here is that when error happens, stacktrace will contain function names instead of anonymous functions. This is really the only purpose of it, it doesn't serve any functional purpose for normal execution.

@kripken
Copy link
Member

kripken commented Mar 7, 2018

I see, thanks. One related option is DEMANGLE_SUPPORT which improves stack traces by demangling C++ names in them. Another related option is the debug level, in -g2 or above, or when --profiling-funcs, we keep function names around. The second is probably closer I think - maybe it's reasonable to use that.

@nazar-pc
Copy link
Contributor Author

nazar-pc commented Mar 7, 2018

DEMANGLE_SUPPORT sounds really close, I'll can make a separate PR later

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