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

BEMHTML: support nested mixes #448

Merged
merged 1 commit into from Jul 18, 2017
Merged

Conversation

miripiruni
Copy link
Contributor

@miripiruni miripiruni commented May 10, 2017

Fix #241

Instead of #280

@miripiruni
Copy link
Contributor Author

miripiruni commented May 10, 2017

screen shot 2017-05-10 at 17 27 54

$ node runner.js --rev1 5aad540b0d97e379372558e688ab9021d44eea9b --rev2 a2d25efb5a93bbf28c8b386dbb9f95c8dc3bc2ca --bemjson 10000 --dataPath ~/bemjson --templatePath ./template-8x.js
Test started…
{ _: [],
  h: false,
  help: false,
  rev1: '5aad540b0d97e379372558e688ab9021d44eea9b',
  rev2: 'a2d25efb5a93bbf28c8b386dbb9f95c8dc3bc2ca',
  bemjson: 10000,
  dataPath: '/Users/miripiruni/bemjson',
  templatePath: './template-8x.js',
  '$0': 'runner.js',
  repeat: undefined,
  verbose: undefined }


Wed May 10 2017 17:19:30 GMT+0300 (MSK)

Total test time:  238615.161181
./lib/compare.py ./dat-5aad5-a2d25 ./dat-5aad5-a2d25/5aad540b0d97e379372558e688ab9021d44eea9b-10000-1494426090791.dat ./dat-5aad5-a2d25/a2d25efb5a93bbf28c8b386dbb9f95c8dc3bc2ca-10000-1494426208718.dat

Percentile:  0.5
{ rev1: 7.719904,
  rev2: 7.520985,
  'diff abs': 0.19891900000000007,
  'diff percent': 2.576703026358873 }

Percentile:  0.9
{ rev1: 10.773798,
  rev2: 10.624067,
  'diff abs': 0.14973099999999917,
  'diff percent': 1.3897698843063466 }

Percentile:  0.95
{ rev1: 12.338903,
  rev2: 12.196022,
  'diff abs': 0.14288100000000092,
  'diff percent': 1.1579716608518642 }

@miripiruni miripiruni changed the title Issue 241 nested mixes 2 BEMHTML: support nested mixes May 10, 2017
@miripiruni
Copy link
Contributor Author

miripiruni commented May 10, 2017

@veged @zxqfox what do you think:

~1% tradeoff (95 percentile) on benchmark — is it OK?

@coveralls
Copy link

coveralls commented May 10, 2017

Coverage Status

Coverage increased (+0.001%) to 99.687% when pulling 1692272 on issue-241__nested-mixes-2 into a2d25ef on master.

@pvdz
Copy link

pvdz commented May 10, 2017

@miripiruni I think any perf trade should be worth the feature being implemented. It's hard to tell whether that is the case here. Furthermore this PR introduces 11k loc and there's no way in hell anybody is going to be able to review this properly, so good luck with that.

I would give more insights but aside from this PR not really being reviewable, I simply don't know enough about this project to begin with even it it was. Perhaps @zxqfox does, though.

@pvdz
Copy link

pvdz commented May 10, 2017

PS. It's impressive that the 11k loc and one test actually improved code coverage by 0.001%. It's in fact so impressive that would suggest somebody take a closer look at that because it's pretty unlikely to be correct.

@miripiruni
Copy link
Contributor Author

miripiruni commented May 10, 2017

@qfox my apologies, I think it’s second time when I confused nickname. That’s because @zxqfox have ‘qfox’ username as corporate login…

You can ban me :)

P..S. 11K loc in bench dir. But coverage watch only lib dir.

@pvdz
Copy link

pvdz commented May 10, 2017

You can ban me :)

shrug that's okay :)

P..S. 11K loc in bench dir. But coverage watch only lib dir.

Ah that makes sense.

@coveralls
Copy link

coveralls commented Jun 2, 2017

Coverage Status

Coverage increased (+0.001%) to 99.687% when pulling 5aad540 on issue-241__nested-mixes-2 into 908c67d on master.

@pvdz
Copy link

pvdz commented Jun 2, 2017

@zxqfox ^

@miripiruni miripiruni merged commit 1641c5b into master Jul 18, 2017
@miripiruni miripiruni deleted the issue-241__nested-mixes-2 branch July 18, 2017 13:39
@miripiruni
Copy link
Contributor Author

bem-xjst@8.7.1

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

3 participants