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

bem-xjst: clean cached flags for wrap() and extend() (fix for #495) #496

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented Jan 16, 2018

Fixes #495

Changes proposed in this pull request

On every templates.apply(bemjson) call we can use copy of bemjson, not the bemjson by link.

Checklist

  • Tests added
  • Benchmark checked
./lib/compare.py ./dat-10550-4ee9b ./dat-10550-4ee9b/10550d5fa9e1cf54edec5b86e30b261b533488c6-2000-1523269017851.dat ./dat-10550-4ee9b/4ee9bbd2059c6e8dc6627785e591bf9fa3a27a06-2000-1523269032181.dat
/bin/sh: gnuplot: command not found
/bin/sh: gnuplot: command not found
Percentile:  0.5
{ rev1: 4.211397,
  rev2: 4.089813,
  'diff abs': 0.12158399999999947,
  'diff percent': 2.8870229997314256 }

Percentile:  0.9
{ rev1: 6.285278,
  rev2: 6.214809,
  'diff abs': 0.07046900000000011,
  'diff percent': 1.1211755470482032 }

Percentile:  0.95
{ rev1: 7.158743,
  rev2: 6.986244,
  'diff abs': 0.17249900000000018,
  'diff percent': 2.4096269414895954 }

@tadatuta if it looks like production decision for you, I can run benchmarks.

@miripiruni
Copy link
Contributor Author

@coveralls
Copy link

coveralls commented Jan 17, 2018

Coverage Status

Coverage increased (+0.005%) to 99.408% when pulling 3b55c9a on issue-495__isolate-input-bemjson into 4ee9bbd on master.

@miripiruni
Copy link
Contributor Author

Travis alive.

@miripiruni
Copy link
Contributor Author

@tadatuta ping

@tadatuta
Copy link
Member

I'm not sure that shallow copy is a good fix :(

For example such case is still broken:

const bemhtml = require('bem-xjst').bemhtml;
const tmpl = bemhtml.compile(function() {
    block('b3').wrap()(node => ({
        block: 'b2',
        content: node.ctx
    }));
});

const bemjson = { block: 'b1', content: { block: 'b3' } };

const firstTime = tmpl.apply(bemjson);
const secondTime = tmpl.apply(bemjson);

console.log('firstTime:', firstTime);
console.log('secondTime:', secondTime);

@miripiruni
Copy link
Contributor Author

@tadatuta OK, that was a quick attempt. The bug is deeply related to https://github.com/bem/bem-xjst/blob/master/lib/bemxjst/match.js#L46-L47 and it’s flag prevent infinity loop. Right now I have no idea how to fix it… :(

Do you have any suggestions?

@tadatuta
Copy link
Member

@zxqfox we discussed possible fix with @miripiruni few days ago.
the idea is to collect pointers to nodes where wrap flag was added and remove the flag for each apply() call. but i'm still not quite sure if it's fine (as it won't fix the issue for manual interaction inside def() mode at least).

@miripiruni miripiruni force-pushed the issue-495__isolate-input-bemjson branch from 95373cc to 5b5aa73 Compare April 9, 2018 09:56
@miripiruni miripiruni force-pushed the issue-495__isolate-input-bemjson branch from 5b5aa73 to 10550d5 Compare April 9, 2018 09:57
@miripiruni miripiruni changed the title bem-xjst: isolate input bemjson (fix for #495) bem-xjst: clean cached flags for wrap() and extend() (fix for #495) Apr 9, 2018
@miripiruni
Copy link
Contributor Author

miripiruni commented Apr 9, 2018

@tadatuta Updated according to your idea.

Benchmark result ~ +2%…

cc @@zxqfox

@tadatuta
Copy link
Member

tadatuta commented Apr 9, 2018

Benchmark result ~ +2%…

2% slower then previous solution with shallow copy?
Looks to much for me.
How many wrap/extend calls are there in bench templates?

@miripiruni
Copy link
Contributor Author

miripiruni commented Apr 9, 2018

@tadatuta

How many wrap/extend calls are there in bench templates?

  1. (and ~450 block() subpredicates). How many times wrap() was matched I don’t know.

2% slower then previous solution with shallow copy?
Looks to much for me.

measurement error ~1%. It’s normal measurement error on my macbook with turned off network.

After optimisation commit results looks better:

rm -fr dat-10550-4ee9b/ && node runner.js --rev1 https://github.com/bem/bem-xjst/pull/496/commits/285637ced9a5b6590805fb93340ea765e70320d0 --rev2 4ee9bbd2059c6e8dc6627785e591bf9fa3a27a06 --bemjson 2000 --dataPath ~/Documents/www/web-data/data --templatePath ./template-8x.js
Percentile:  0.5
{ rev1: 4.059073,
  rev2: 3.991944,
  'diff abs': 0.06712899999999955,
  'diff percent': 1.6538012496941978 }

Percentile:  0.9
{ rev1: 6.045441,
  rev2: 6.015059,
  'diff abs': 0.030382000000000353,
  'diff percent': 0.5025605245341125 }

Percentile:  0.95
{ rev1: 7.035301,
  rev2: 6.961316,
  'diff abs': 0.07398499999999952,
  'diff percent': 1.0516252254167857 }

I tend to think of this as an insignificant speed change.

@miripiruni miripiruni force-pushed the issue-495__isolate-input-bemjson branch from f3eb923 to 285637c Compare April 9, 2018 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants