bem-xjst icon indicating copy to clipboard operation
bem-xjst copied to clipboard

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

Open miripiruni opened this issue 7 years ago • 10 comments

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

  • [x] Tests added
  • [x] 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 avatar Jan 16 '18 17:01 miripiruni

https://www.traviscistatus.com/incidents/c1s4dlyxd4lf

miripiruni avatar Jan 16 '18 20:01 miripiruni

Coverage Status

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

coveralls avatar Jan 17 '18 10:01 coveralls

Travis alive.

miripiruni avatar Jan 18 '18 13:01 miripiruni

@tadatuta ping

miripiruni avatar Jan 18 '18 13:01 miripiruni

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);

tadatuta avatar Jan 18 '18 17:01 tadatuta

@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?

miripiruni avatar Jan 18 '18 17:01 miripiruni

@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).

tadatuta avatar Jan 24 '18 20:01 tadatuta

@tadatuta Updated according to your idea.

Benchmark result ~ +2%…

cc @@zxqfox

miripiruni avatar Apr 09 '18 10:04 miripiruni

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?

tadatuta avatar Apr 09 '18 10:04 tadatuta

@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 avatar Apr 09 '18 11:04 miripiruni