Opened on 01/17/2017 at 04:30:47 PM

Last modified on 04/05/2017 at 08:55:47 AM

#4812 new change

Use a string builder interface in `WebRequest::GET()`

Reported by: eric@adblockplus.org Assignee:
Priority: P5 Milestone:
Module: Libadblockplus Keywords:
Cc: sergz, fhd Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

WebRequest::GET() returns the response text as a string inside of a response structure. This means that the response is (1) first written to a C++ string and (2) then converted to a v8 string. This doesn't matter a lot for small strings, but for around 2 MB of filters it's an unnecessary overhead, both in performance and memory, particularly on mobile where memory is more constrained.

What to change

Create some kind of string builder interface to pass to WebRequest::GET(). Implement this interface for v8 strings so that we can avoid putting the response into any C++ string. This mechanism avoids incurring a direct v8 dependency in WebRequest.

For development, implement a string builder for C++ strings. Early on, this will allow separating work on platform-specific WebRequest implementations from work on the cross-platform WebRequestTask which calls GET(). Later on, this can be used in unit tests.

Attachments (0)

Change History (1)

comment:1 Changed on 04/05/2017 at 08:55:47 AM by sergz

  • Cc sergz fhd added
  • Priority changed from Unknown to P5

I doubt that it is a real performance issue and, I guess, won't be an issue for at least near future even when the size of response is a couple if times bigger, like 10MB. So I would propose to close this issue.

First of all, a couple or several MBs C++ strings are not a problem even on mobile platform. Secondly, C++ string is prepared in a background thread and does not significantly influences performance and in particular UX, however conversion to v8::String requires blocking of execution of JavaScript what negatively affects UX and it seems string builder here will be less efficient than a C++ string because accessing of a continuous sequence of bytes is faster than accessing of different parts of memory, it's especially sensible on mobile platforms.

Add Comment

Modify Ticket

Change Properties
Action
as new .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from (none).
Next status will be 'reviewing'.
 
Note: See TracTickets for help on using tickets.