Skip to content
Snippets Groups Projects
  1. Apr 22, 2019
  2. Apr 21, 2019
    • Daniel Martí's avatar
      try to deflake TestExecAllocatorCancelParent again · a601b530
      Daniel Martí authored
      This time, the theory is that Travis is lucky enough to have the page
      load before the browser is finally killed.
      
      To get rid of this possibility, make the server not respond until the
      test finishes. This also simplifies the code a bit.
      a601b530
    • Daniel Martí's avatar
      have RemoteAllocator cancel on a dropped ws connection · 2130fcaa
      Daniel Martí authored
      ExecAllocator did this already, but not RemoteAllocator. It was a bit
      tricky, as its Allocate happens with a completely separate context, so
      we can delay closing the websocket on cancel until the pages have been
      closed.
      
      To work around the issue, signal that the browser connection has been
      lost via a channel field on Browser, and not via Context. This works
      even when the context used in Allocate doesn't contain anything related
      to chromedp.
      
      This also means that Allocator implementations can have the same feature
      from third party packages, as the LostConnection field is exported.
      2130fcaa
    • Daniel Martí's avatar
      make RemoteAllocator close pages on cancel · b2c215d5
      Daniel Martí authored
      To do this, we need to use a separate context for the websocket and
      browser, so that we can send the appropriate commands like Page.close.
      
      The code in RemoteAllocator is a bit hard-coded with the chromedp
      internals right now, but that can be improved later.
      b2c215d5
    • Daniel Martí's avatar
      first implementation of RemoteAllocator · dd6605f8
      Daniel Martí authored
      It doesn't close pages correctly yet (see the TODO), but at least the
      basics work.
      dd6605f8
    • Kenneth Shaw's avatar
      Change to Transport/Conn types · 3a611e2a
      Kenneth Shaw authored
      Changes made to the Transport and Conn types to accommodate better
      testing of alternate websocket implementations.
      3a611e2a
  3. Apr 20, 2019
    • Kenneth Shaw's avatar
      Updating travis config · 816b3272
      Kenneth Shaw authored
      The CHROMEDP_DEBUG option introduced in last commit causes travis to
      cancel the build, since the log output is too long. This removes the
      environment variable.
      816b3272
    • Kenneth Shaw's avatar
      Adds CHROMEDP_DEBUG option to tests · 6b8480d0
      Kenneth Shaw authored
      In TestMain, adds recognition of CHROMEDP_DEBUG environment
      variable that when non-empty and not set to "false", adding
      `WithDebugf(log.Printf)` option to the Browser context for tests.
      6b8480d0
    • Daniel Martí's avatar
      remove an easyjson alloc per target event · 95473d10
      Daniel Martí authored
      By reusing the lexer used to decode the nested object. While at it,
      deduplicate the custom easyjson.Unmarshal implementation code.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    27.4ms ± 5%    27.3ms ± 6%    ~     (p=0.912 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     344kB ± 0%     344kB ± 0%    ~     (p=0.165 n=10+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8       990 ± 1%       952 ± 1%  -3.89%  (p=0.000 n=10+10)
      95473d10
    • Daniel Martí's avatar
      reuse the easyjson lexer when decoding events too · 10363ae5
      Daniel Martí authored
      name           old time/op    new time/op    delta
      TabNavigate-8    27.3ms ± 3%    27.4ms ± 5%    ~     (p=0.796 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     349kB ± 0%     344kB ± 0%  -1.34%  (p=0.000 n=9+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.03k ± 0%     0.99k ± 1%  -3.83%  (p=0.000 n=8+10)
      10363ae5
    • Daniel Martí's avatar
      run a single time.Ticker per Browser · 037ae6c5
      Daniel Martí authored
      Instead of one per Target, which adds a non-trivial amount of overhead
      once we have many targets on a single browser.
      
      The reduced overhead also means we can tick more often and gain a bit of
      performance. This results in a slight increase in allocs, since some
      wait functions allocate, but what matters is the elapsed time.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    28.0ms ± 4%    27.3ms ± 3%  -2.44%  (p=0.043 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     346kB ± 0%     349kB ± 0%  +1.05%  (p=0.000 n=10+9)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8       985 ± 0%      1030 ± 0%  +4.49%  (p=0.000 n=10+8)
      037ae6c5
    • Daniel Martí's avatar
      remove detached sessions from Browser · fd478771
      Daniel Martí authored
      Before this, it was possible to create and cancel targets on a browser,
      and the map[target.SessionID]*Target would keep growing without ever
      shrinking back down.
      
      This wasn't caught by any of the tests or benchmarks though, as the map
      was initialized with a size of 1024 and the benchmark was usually run
      with N=150.
      
      In any case, this implementation is more correct, doesn't leak memory,
      and will scale better. No noticeable benchmark changes.
      fd478771
    • Daniel Martí's avatar
      simplify adding new tabs to Browser · fdbedd83
      Daniel Martí authored
      A single channel is enough. It's also better to set up Target and start
      its goroutine from newExecutorForTarget, as we don't take up resources
      on the goroutine that handles routing target events.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    29.3ms ± 5%    28.4ms ± 4%  -2.90%  (p=0.043 n=10+10)
      fdbedd83
    • Daniel Martí's avatar
      try to fix the TestBrowserQuit flake on Travis · 50901aba
      Daniel Martí authored
      It would sometimes fail with the "did not expect a nil error" failure.
      Intuitively, this could happen if Chrome takes too long to respond to
      os.Interrupt, and actually succeeds at loading the page.
      
      To make that impossible, send it os.Kill, which should exit the process
      immediately. If the flakes still happen, we can consider adding an extra
      wait on the process, or some form of sleep.
      
      Another bonus is that os.Kill works on Windows, so the test is now
      portable.
      50901aba
    • Daniel Martí's avatar
      fix WithDebugf after a regression in 9ca47714 · d6fc931c
      Daniel Martí authored
      The easyjson Buffer type doesn't allow being dumped twice. That is, the
      BuildBytes call empties the buffer before returning, so the DumpTo call
      later wasn't writing anything.
      
      If we need to print to dbgf, use those bytes from BuildBytes directly to
      send to the websocket.
      d6fc931c
    • Daniel Martí's avatar
      remove one chan send from Target.Execute · d66dd6a4
      Daniel Martí authored
      We can refactor cmdJob a bit to remove the need for two channel sends.
      They aren't trivially cheap, and the old logic was a bit overly complex.
      
      While at it, initialise cmdQueue as a buffered channel, to avoid
      unnecessary locking in Target.Execute. Do the same for an errc which is
      often used for only one error.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    28.5ms ± 6%    28.8ms ± 6%    ~     (p=0.552 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     347kB ± 0%     346kB ± 0%  -0.42%  (p=0.001 n=9+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.00k ± 1%     0.98k ± 1%  -1.78%  (p=0.000 n=9+10)
      d66dd6a4
    • Daniel Martí's avatar
      rework Conn.Read to not require an allocation · 9e8d66aa
      Daniel Martí authored
      For example, EventTargetReceivedMessageFromTarget is a message wrapping
      another message. The wrapping message is thrown away, so there's no need
      to allocate a new one every time we receive that event.
      
      That kind of event is very common, so this rewrite saves a noticeable
      amount of allocs.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    28.4ms ± 8%    28.5ms ± 6%    ~     (p=0.739 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     350kB ± 0%     347kB ± 0%  -0.88%  (p=0.000 n=10+9)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.03k ± 0%     1.00k ± 1%  -3.69%  (p=0.000 n=10+9)
      9e8d66aa
    • Daniel Martí's avatar
      reuse jwriter.Writer when writing to the websocket · 9ca47714
      Daniel Martí authored
      Removes an allocation per written message. Like in the Read case, this
      also lets us deduplicate the dbgf code.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    29.0ms ± 5%    28.4ms ± 8%    ~     (p=0.190 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     352kB ± 0%     350kB ± 0%  -0.69%  (p=0.000 n=10+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.06k ± 1%     1.03k ± 0%  -2.03%  (p=0.000 n=10+10)
      9ca47714
    • Daniel Martí's avatar
      reuse an easyjson lexer in Conn.Read · 569afa6f
      Daniel Martí authored
      This function is called very often, so reducing one allocation per call
      is very significant and doesn't cost much code complexity.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    28.8ms ± 9%    29.0ms ± 5%    ~     (p=0.579 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     359kB ± 0%     352kB ± 0%  -1.96%  (p=0.000 n=9+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.11k ± 1%     1.06k ± 1%  -4.95%  (p=0.000 n=10+10)
      569afa6f
    • Daniel Martí's avatar
      avoid using easyjson.UnmarshalFromReader · 328490dc
      Daniel Martí authored
      It uses ioutil.ReadAll, which doesn't reuse space. Rolling our own
      results in a pretty massive reduction in allocations.
      
      This also means we can deduplicate code with Conn.dbgf.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    29.3ms ± 6%    28.8ms ± 9%     ~     (p=0.280 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     486kB ± 1%     359kB ± 0%  -26.11%  (p=0.000 n=10+9)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.21k ± 2%     1.11k ± 1%   -8.26%  (p=0.000 n=10+10)
      328490dc
    • Daniel Martí's avatar
      fix data race in ExecAllocator.Allocate · a095b367
      Daniel Martí authored
      We can't use cmd in the cancellation goroutine before setting up cmd;
      that's racey. 'go test -race' rightfully pointed this out:
      
      	Write at 0x00c000010080 by goroutine 33:
      	  github.com/chromedp/chromedp.(*ExecAllocator).Allocate()
      	      /home/mvdan/src/chromedp/allocate.go:141 +0xb1a
      	  github.com/chromedp/chromedp.Run()
      	      /home/mvdan/src/chromedp/chromedp.go:178 +0x2a8
      	  github.com/chromedp/chromedp.TestPrematureCancel()
      	      /home/mvdan/src/chromedp/chromedp_test.go:208 +0xc3
      	  testing.tRunner()
      	      /home/mvdan/tip/src/testing/testing.go:865 +0x162
      
      	Previous read at 0x00c000010080 by goroutine 93:
      	  github.com/chromedp/chromedp.(*ExecAllocator).Allocate.func1()
      	      /home/mvdan/src/chromedp/allocate.go:120 +0x67
      a095b367
    • Daniel Martí's avatar
      reduce allocs for each received target event · 724a35d4
      Daniel Martí authored
      We receive a lot of events which are encapsulated for each target.
      
      Unfortunately, the actual event message is received as a string, which
      we then convert to []byte and unmarshal again.
      
      This means many allocations to unmarshal each of these event messages. A
      better approach is to perform a single unmarshal, decoding the
      underlying message directly from the input bytes.
      
      As expected, this results in a significant reduction in allocations.
      There's a tiny slowdown in comparison, but nothing significant given the
      high variance.
      
      encoding/json has the ",string" option precisely for this purpose.
      Unfortunately, easyjson doesn't have it, so roll our own with a custom
      type.
      
      In the future, I'd like to see this type in cdproto, but for now it can
      live here. While at it, slightly simplify some types.
      
      ok      github.com/chromedp/chromedp    47.134s
      name           old time/op    new time/op    delta
      TabNavigate-8    28.5ms ± 6%    29.3ms ± 6%    ~     (p=0.218 n=10+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     498kB ± 1%     486kB ± 1%  -2.30%  (p=0.000 n=10+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.32k ± 1%     1.21k ± 2%  -7.91%  (p=0.000 n=10+10)
      724a35d4
    • Daniel Martí's avatar
      try to resolve wait funcs earlier · 0eb505a1
      Daniel Martí authored
      The old codebase ran them approximately every 5ms via sleeps. This had
      two major problems.
      
      First, we blocked the Target.run goroutine, meaning that we wouldn't be
      able to receive more events until after those 5ms.
      
      Second, the sleep would be a waste of time in some scenarios. For
      example, waiting for a node to be ready is a common occurrence, but it
      can very often be finished right after the next received DOM event.
      
      To fix the first issue, use a ticker, which doesn't stop our goroutine
      from doing other work like handling events.
      
      To fix the second, check the wait funcs after every DOM event. This does
      mean that we run the funcs more often, which allocate some objects, but
      this is still a net win in wall time.
      
      To help reduce the allocations a bit, remove the need for waitReady to
      allocate a []error and a WaitGroup every time it is run.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    31.3ms ± 2%    28.5ms ± 6%   -8.72%  (p=0.000 n=8+10)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     451kB ± 0%     498kB ± 1%  +10.32%  (p=0.000 n=9+10)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.03k ± 1%     1.32k ± 1%  +27.91%  (p=0.000 n=10+10)
      0eb505a1
    • Daniel Martí's avatar
      use easyjson marshal/unmarshal APIs more often · c2c2da6b
      Daniel Martí authored
      Doesn't particularly give us a CPU win, but it does noticeably reduce
      allocations.
      
      name           old time/op    new time/op    delta
      TabNavigate-8    30.9ms ± 3%    31.3ms ± 2%     ~     (p=0.083 n=10+8)
      
      name           old alloc/op   new alloc/op   delta
      TabNavigate-8     458kB ± 0%     451kB ± 0%   -1.42%  (p=0.000 n=10+9)
      
      name           old allocs/op  new allocs/op  delta
      TabNavigate-8     1.19k ± 0%     1.03k ± 1%  -13.49%  (p=0.000 n=10+10)
      c2c2da6b
    • Daniel Martí's avatar
      add the first simple benchmark · f831182a
      Daniel Martí authored
      This parallel benchmark simply opens a new tab on a browser, navigates
      to a local page, and waits for an element to be visible. Will be useful
      to monitor chromedp's overhead, for example when it comes to marshaling
      and unmarshaling json objects when talking to Chrome.
      
      name           time/op
      TabNavigate-8  30.9ms ± 3%
      
      name           alloc/op
      TabNavigate-8   458kB ± 0%
      
      name           allocs/op
      TabNavigate-8   1.19k ± 0%
      f831182a
  4. Apr 18, 2019
  5. Apr 17, 2019
  6. Apr 16, 2019
  7. Apr 14, 2019
    • Daniel Martí's avatar
      error in Run if passed an allocator context · ac47d6ba
      Daniel Martí authored
      The correct way to use Run is after NewContext. Using NewExecAllocator
      only, which also gives a context, almost worked. This confused some
      users into thinking it was a bug in chromedp.
      
      Instead, error immediately with an invalid context error.
      
      Fixes #299.
      ac47d6ba
  8. Apr 09, 2019
    • Daniel Martí's avatar
      92a77355
    • Daniel Martí's avatar
      rework CancelError into Cancel · 46982a1c
      Daniel Martí authored
      That way, cancelling a context while checking the error is much simpler.
      The context data already holds onto the cancel func, so this requires no
      internal changes.
      
      This renders the Browser.Shutdown API obsolete, even though it doesn't
      do exactly the same. If we want Cancel to do a proper shutdown action
      before cancelling a browser context and killing the process, we could do
      that change within the cancel logic.
      46982a1c
    • Kenneth Shaw's avatar
      Add WithDebugf() context option · e8122e4a
      Kenneth Shaw authored
      Adds the high level WithDebugf() context option, and associated lower
      level browser and dial options for setting a protocol wire debugger.
      Additionally changes the conn.Conn.Read/Write implementations to be more
      efficient, using direct easyjson.{Marshal,Unmarshal} calls and logging
      to debug func when available.
      e8122e4a
  9. Apr 08, 2019
    • Daniel Martí's avatar
      support fetching errors from cancellation · b481eeac
      Daniel Martí authored
      The API isn't very shiny, but it works. It doesn't matter that much, as
      most users won't care about these errors.
      
      Fixes #295.
      b481eeac
    • Daniel Martí's avatar
      support using BrowserOption in NewContext · b8efcf06
      Daniel Martí authored
      While at it, remove the error return from BrowserOption, to make it
      consistent with all the other option func types.
      
      We don't have a good test for this feature yet, but at least we check
      that it doesn't crash or error via an example.
      
      Fixes #292.
      b8efcf06
    • Daniel Martí's avatar
      deflake TestNavigate · a29b1ec1
      Daniel Martí authored
      For some reason, this test fails about half the time on Travis, but I
      can't get it to fail even after stress-testing hundreds of concurrent
      runs.
      
      It might be because Travis is on a much older version of Chrome. We'll
      fix that soon, by having chromedp select a specific version of Chrome.
      
      For now, make it more conservative, not assuming that a Location after a
      Navigate isn't racy.
      a29b1ec1
Loading