search

garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.79k stars 604 forks source link

Chromy engine runs slow with scenarios containing many selectors #480

Closed daveaspinall closed 7 years ago

daveaspinall commented 7 years ago

Hi Garris,

Creating a ticket in reference to #479.

Running backstop reference while using "engine": "chromy" takes forever to finish (not actually had it complete yet), whereas phantomjs finishes in 15s.

Backstop version: BackstopJS v3.0.4 Node version: v7.6.0

Snapshot of the backstop.json:

  "viewports": [
    {
      "name": "mobile",
      "width": 375,
      "height": 564
    },
    {
      "name": "tablet",
      "width": 768,
      "height": 1024
    },
    {
      "name": "desktop",
      "width": 1240,
      "height": 1024
    }
  ],
  "scenarios": [
    {
      "label": "Patternlab",
      "url": "Patternlab/public/styleguide/html/styleguide.html",
      "hideSelectors": [
        "[data-shop-map]",
        "[data-search-map]"
      ],
      "removeSelectors": [
        ".sg-pattern-head",
        ".sg-pattern-extra"
      ],
      "selectorExpansion": true,
      "selectors": [
        ".sg-pattern"
      ],
      "readyEvent": null,
      "delay": 500,
      "misMatchThreshold": 0.1,
      "requireSameDimensions": true
    }
  ],
  "engine": "chromy",
  "report": ["browser"]

Just an idea of how long it takes (there are ~54 components on the page): gif

Console output when using "engine": "chromy":

BackstopJS v3.0.4
Loading config:  /Users/daveaspinall/Sites/oxfam/oxfam-main/backstop.json 

COMMAND | Executing core for `reference`
  clean | backstop_data/bitmaps_reference was cleaned.
createBitmaps | Selcted 1 of 1 scenarios.
Starting Chromy: port:9222 [ '--window-size=375,564' ]
CREATING NEW REFERENCE FILES
Starting Chromy: port:9223 [ '--window-size=768,1024' ]
CREATING NEW REFERENCE FILES
Starting Chromy: port:9224 [ '--window-size=1240,1024' ]
CREATING NEW REFERENCE FILES
9222 INFO >  backstopTools are running
9223 INFO >  backstopTools are running
9224 INFO >  backstopTools are running

Run `$ backstop test` to generate diff report.

      COMMAND | Command `reference` sucessfully executed in [2143.046s]

Console output when using "engine": "phantomjs":

BackstopJS v3.0.4
Loading config:  /Users/daveaspinall/Sites/oxfam/oxfam-main/backstop.json 

COMMAND | Executing core for `reference`
  clean | backstop_data/bitmaps_reference was cleaned.
createBitmaps | Selcted 1 of 1 scenarios.

Running CasperJS with:  [ '~/.nvm/versions/node/v7.6.0/lib/node_modules/backstopjs/capture/genBitmaps.js',
  '--captureConfigFileName=/var/folders/44/1j9_33pj2kj0ygpsg03jmk_chh6558/T/capture/faaa966138ec0284c589c66a65cb1ca2ffc37025.json' ]
CasperJS:  CREATING NEW REFERENCE FILES
CasperJS:  Ready event received.
CasperJS:  Current location is Patternlab/public/styleguide/html/styleguide.html
CasperJS:  Capturing screenshots for mobile (375x564)
CasperJS:  Ready event received.
CasperJS:  Current location is Patternlab/public/styleguide/html/styleguide.html
CasperJS:  Capturing screenshots for tablet (768x1024)
CasperJS:  Ready event received.
CasperJS:  Current location is Patternlab/public/styleguide/html/styleguide.html
CasperJS:  Capturing screenshots for desktop (1240x1024)
CasperJS:  Comparison config file updated.

Bitmap file generation completed.

Run `$ backstop test` to generate diff report.

      COMMAND | Command `reference` sucessfully executed in [15.859s]

Any ideas what could be slowing this down? Let me know if you need any more info and I'll post it up.

Thanks 🤓

dotneet commented 7 years ago

Hi @daveaspinall, I'm owner of chromy. Could you tell me your chrome version? If you use canary version, please update it to latest version and try out again.

daveaspinall commented 7 years ago

Hey @dotneet!

Sure, versions for both stable Chrome and Canary below:

Chrome: Version 59.0.3071.115 Canary: Version 61.0.3163.0

Just wanted to say I'm actually really enjoying Chromy (we use it on some other tests) and thanks for the brilliant work 😇

daveaspinall commented 7 years ago

Update: Backstop is using Canary (which is apparently up to date) but it's still very slow

dotneet commented 7 years ago

@daveaspinall Thanks! I found the cause of this problem.

@garris Please update chromy version. chromy ver.0.3.3 doesn't work with chrome61 (canary). 0.3.4 or later works.

daveaspinall commented 7 years ago

@dotneet Thanks for the speedy solution!

garris commented 7 years ago

@dotneet Updated!

daveaspinall commented 7 years ago

@dotneet @garris: Updated the dependencies and tried the reference again but it's no faster I'm afraid 😕

npm view backstopjs@3.0.5 dependencies

{ casperjs: '^1.1.0-beta5',
  chalk: '^1.1.3',
  chromy: '^0.3.6',
  'fs-extra': '^0.30.0',
  junitwriter: '~0.3.1',
  minimist: '^1.2.0',
  'node-resemble-js': '^0.2.0',
  'object-hash': '1.1.5',
  open: '0.0.5',
  os: '^0.1.1',
  'p-map': '^1.1.1',
  'phantomjs-prebuilt': '^2.1.7',
  sharp: '^0.18.1',
  sinon: '^1.17.7',
  temp: '^0.8.3' }
dotneet commented 7 years ago

@garris Thank you for updating.

@daveaspinall In my environment, I've confirmed that the problem that I found in BackstopJS 3.0.4 is fixed. But your problem seems to be different with the problem I found. As @garris mentioned in #479, the optimization for selector expansion might be the solution.

garris commented 7 years ago

@daveaspinall Sorry to take so long to reply. Can you pull down this file and run the main backstop test in there? Then include the console output. At the end of the log you'll find a line that looks like this... COMMAND | Command test sucessfully executed in [35.681s] That timestamp is what I am after. Thanks!

garris commented 7 years ago

@daveaspinall almost forgot to add the repo... https://github.com/garris/backstop-feature-tests

^^^ please let me know the test time for this thanks!

daveaspinall commented 7 years ago

@garris no problem! Appreciate the help from both of you on this.

Ran the test and it looks much faster (although still nowhere near the 15s of phantomjs)! Console output below:

COMMAND | Command `reference` sucessfully executed in [43.171s]
COMMAND | Command `openReport` sucessfully executed in [0.063s]
COMMAND | Command `report` sucessfully executed in [0.175s]
COMMAND | Command `test` sucessfully executed in [42.062s]

There must be an issue with my specific config. Going to copy the backstop.json over from these tests and see if that helps.

Update: copied the config over to my project and removed all but the isExpanded scenario:

COMMAND | Command `reference` sucessfully executed in [443.649s]
garris commented 7 years ago

I assume the same thing happens if you run test instead of reference? Also, how many selectors is backstop capturing.

There is an optimization planned where multiple selectors would only require much less rendering work -- this would probably make a very big difference in your case.

garris commented 7 years ago

Ok. I think this is going to be important. I will refactor our isExpanded code path. It could be some time -- but it is the next thing on my list. Thanks for your patience.

garris commented 7 years ago

@daveaspinall are you able to provide me access to the web app which causes the issue. I would like to benchmark the issue.

daveaspinall commented 7 years ago

@garris apologies for the late reply!

There is an optimization planned where multiple selectors would only require much less rendering work -- this would probably make a very big difference in your case.

This may help, but I've also tried the reference on only 3 selectors and it still ran slow?

@daveaspinall are you able to provide me access to the web app which causes the issue. I would like to benchmark the issue.

Happy to set up my project for you to test! You may inadvertently find a problem with the setup. I'll send you a link shortly 🙂

daveaspinall commented 7 years ago

@garris test project setup for you:

https://github.com/daveaspinall/backstop-patternlab-test

Running backstop reference on in this test on my machine takes [428.798s]

garris commented 7 years ago

Great. Thank you!

garris commented 7 years ago

It is looking like it may be a flaw with the expandedSelectors feature and not necessarily with the count of selectors. This issue is now my top priority.

Chromy has native support for this feature-- I will attempt to use that.

daveaspinall commented 7 years ago

@garris as I've said before, I really appreciate you putting this issue high priority! 😇

dotneet commented 7 years ago

@garris I've published chromy v0.4.0 that includes some performance improvements that can be used with chrome61 or later. And I confirmed that this improvements soften this problem.

NOTE: this version requires chrome60 or later. to get performance improvements using chrome canary is recommended.

dotneet commented 7 years ago

In addition, new .screenshotSelector and .screenshotMultipleSelectors can get around the problem that be caused on very long page.

garris commented 7 years ago

@dotneet that is excellent! Can't wait to try it out!

(I am still working to refactor the multiple selector code path)

garris commented 7 years ago

@dotneet Just tested this.

Yes speed is much better... (Even before my refactor work) But I think there is a chromy regression -- images are double resolution.

previous version Command test sucessfully executed in [19.099s]

image

0.4.0 version Command test sucessfully executed in [12.424s]

image

dotneet commented 7 years ago

Sorry, I've update chromy to v0.4.2 because of a regression bug.

daveaspinall commented 7 years ago

@garris @dotneet great! I'll have a play tomorrow!

garris commented 7 years ago

@dotneet this is a big improvement-- thank you! I hope to finish my refactor in the next day or so also.

garris commented 7 years ago

UPDATE: This is almost fixed -- I just need to do some cleanup work before pushing a new version. Early indication is that speed way better!

garris commented 7 years ago

Ok. New version. Hopefully this issue is sufficiently fixed...

old perf Command test successfully executed in [19.099s]

new perf Command test successfully executed in [11.762s]

tested on isExpanded scenario in backstop test repo.

garris commented 7 years ago

@daveaspinall Please retest to see if that addresses the issue. Thanks!

remember to npm install -g backstopjs@beta

kiran-redhat commented 7 years ago

shouldn't it be 'npm install backstop@beta' ? and can't we run using locally installed backstop( without -g)?

On Aug 1, 2017 4:01 PM, "Garris" notifications@github.com wrote:

@daveaspinall https://github.com/daveaspinall Please retest to see if that addresses the issue. Thanks!

remember to npm install -g backstopjs

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/garris/BackstopJS/issues/480#issuecomment-319261412, or mute the thread https://github.com/notifications/unsubscribe-auth/AQUrIyoHttFEeY86cwdn4zu45GX8mYK0ks5sTqL-gaJpZM4OfGwV .

garris commented 7 years ago

@kiran-redhat 😮 Yes! Great catch! I fixed the line above.

And yes. Local install should still work fine.

Thank you for posting!

daveaspinall commented 7 years ago

@garris Can confirm the reference task is much much faster!

Was:

COMMAND | Command `reference` sucessfully executed in [2143.046s]

Now:

COMMAND | Command `reference` sucessfully executed in [35.389s]

Seriously great work!! 🙌🏼 Going to mark this issue as complete 🎉

Now to fix our issue with Chromy in my Docker container and our setup with be sweet

garris commented 7 years ago

Ok. I think this is addressed for now.

Closing -- but please reopen if your problem is not resolved.

Cheers.