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

Ref to function self should not create new object #2632

Conversation

legendecas
Copy link
Contributor

@legendecas legendecas commented Dec 5, 2018

Picked from yodaos-project/ShadowNode#368

JerryScript-DCO-1.0-Signed-off-by: legendecas legendecas@gmail.com

@LaszloLango
Copy link
Contributor

Please add a test for this as well.

@zherczeg
Copy link
Member

zherczeg commented Dec 5, 2018

I am also thinking about fixing this. I don't want to add a new argument for vm_run. We need to figure out something better.

@LaszloLango
Copy link
Contributor

Measurement results (RPi2):

Benchmark Heap (bytes) Stack (bytes) Perf (sec)
3d-cube.js 16376 -> 16376 : 0.000% 15888 -> 15896 : -0.050% 0.813 -> 0.814 : -0.046%
3d-raytrace.js 121600 -> 121600 : 0.000% 2840 -> 2840 : 0.000% 1.049 -> 1.053 : -0.398%
access-binary-trees.js 32752 -> 32752 : 0.000% 3304 -> 3304 : 0.000% 0.578 -> 0.576 : +0.390%
access-fannkuch.js 4968 -> 4968 : 0.000% 1584 -> 1592 : -0.505% 2.119 -> 2.124 : -0.249%
access-nbody.js 6248 -> 6248 : 0.000% 1560 -> 1568 : -0.513% 1.089 -> 1.096 : -0.642%
bitops-3bit-bits-in-byte.js 704 -> 704 : 0.000% 1528 -> 1536 : -0.524% 0.508 -> 0.510 : -0.365%
bitops-bits-in-byte.js 656 -> 656 : 0.000% 1528 -> 1536 : -0.524% 0.666 -> 0.665 : +0.207%
bitops-bitwise-and.js 496 -> 496 : 0.000% 1304 -> 1312 : -0.613% 0.925 -> 0.926 : -0.122%
bitops-nsieve-bits.js 98304 -> 98304 : 0.000% 1528 -> 1536 : -0.524% 1.142 -> 1.138 : +0.295%
controlflow-recursive.js 888 -> 888 : 0.000% 69784 -> 69784 : 0.000% 0.366 -> 0.366 : -0.063%
crypto-aes.js 33056 -> 33056 : 0.000% 2416 -> 2424 : -0.331% 0.883 -> 0.887 : -0.482%
crypto-md5.js 106488 -> 106488 : 0.000% 1924 -> 1924 : 0.000% 0.604 -> 0.604 : +0.073%
crypto-sha1.js 65528 -> 65528 : 0.000% 1640 -> 1648 : -0.488% 0.596 -> 0.598 : -0.322%
date-format-tofte.js 16376 -> 16376 : 0.000% 2056 -> 2072 : -0.778% 0.764 -> 0.754 : +1.255%
date-format-xparb.js 16376 -> 16376 : 0.000% 2496 -> 2512 : -0.641% 0.523 -> 0.524 : -0.210%
math-cordic.js 1776 -> 1776 : 0.000% 1528 -> 1536 : -0.524% 1.175 -> 1.184 : -0.778%
math-partial-sums.js 1232 -> 1232 : 0.000% 1488 -> 1496 : -0.538% 0.736 -> 0.737 : -0.145%
math-spectral-norm.js 6088 -> 6088 : 0.000% 1596 -> 1596 : 0.000% 0.539 -> 0.539 : -0.001%
string-base64.js 90792 -> 90792 : 0.000% 1560 -> 1568 : -0.513% 1.409 -> 1.400 : +0.634%
string-fasta.js 16376 -> 16376 : 0.000% 1512 -> 1520 : -0.529% 1.371 -> 1.373 : -0.138%
Geometric mean: 0.000% -0.379% -0.054%
Binary (bytes) 3f9dd0f 42ba64c Gain
size 132664 132664 0.000%
.text 116540 116556 -0.014%
.rodata 11353 11353 0.000%
.bss 1575168 1575168 0.000%

Picked from yodaos-project/ShadowNode#368

JerryScript-DCO-1.0-Signed-off-by: legendecas legendecas@gmail.com
@zherczeg
Copy link
Member

zherczeg commented Dec 6, 2018

I have bad news. We have to do this literally according to spec. See this example:

var f = function g() {
  console.log(g);
  eval("var g = 6");
  console.log(g);
}
f();

@legendecas
Copy link
Contributor Author

legendecas commented Dec 6, 2018

@zherczeg: I have bad news. We have to do this literally according to spec. See this example:

var f = function g() {
  console.log(g);
  eval("var g = 6");
  console.log(g);
}
f();

Looks like the issue still exists with this PR. If we're going to address the issue in this PR?

@zherczeg
Copy link
Member

zherczeg commented Dec 6, 2018

Started to put together a correct solution #2634 . Not completed yet.

@akosthekiss
Copy link
Member

Can we close this in favour of #2634 ? It seems to be close to completion.

@LaszloLango
Copy link
Contributor

@akosthekiss yes we can close this, #2634 is the right fix for the original issue. This PR is just a workaround which does not work correctly on every testcase.

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.

None yet

4 participants