In the meantime, I've contacted floitsch (maintainer of the standalone library), and he'd be interested in consolidating. He confirmed that JavaScript compatibility is a goal, and is willing to incorporate changes we might require, like a BUILD.gn file.
So, since no further concerns have been raised, I think it would make sense to try to work towards pulling the external library into V8; maybe as a copy first and DEPS'ed in later. Specifically, I think that would mean: 1. Use the existing API of the external library, rather than designing our own. If needed, make changes to it that we can upstream. 2. Within V8, move it to its own directory (maybe third_party/double-conversion/), so that comparing it to upstream is easy now and DEPS'ing it later (if we choose to do so) will be a transparent change. 3. Identify any V8-side changes of the past couple of years and upstream them (example: https://github.com/v8/v8/commit/348cc6f152dbcb2d49a45aa3de32a87defcb127c#diff-c1860324f59a0d24623ff41377414164, there may be others) 4. Adapt V8 to use the external library's API, notably including: 4a. Figure out how best to build it. Have BUILD.gn in V8, or upstreamed? Maybe upstream a .gni or something containing just a list of files? 4b. Make sure the V8-side tests keep working -- and passing ;-) 4c. Keep a close eye on performance, just in case any of the upstream changes since forking are causing regressions. (Off the top of my head I don't know which benchmarks in particular to watch.) Unfortunately I don't really have time to help with the legwork in the near future, but you can send the review(s) and/or any questions my way. On Tue, Sep 3, 2019 at 10:42 PM <[email protected]> wrote: > Sorry about not responding earlier, Groups didn’t send the replies to me. > Trying from the web now… > > On Thursday, August 29, 2019 at 9:32:24 AM UTC-4, Nico Weber wrote: >> >> On Thu, Aug 29, 2019 at 6:33 AM Jakob Kummerow <[email protected]> >> wrote: >> >>> I'll give this a shot: >>> >>> On Thu, Aug 29, 2019 at 12:43 AM Robert Sesek <[email protected]> >>> wrote: >>> >>>> I’m replacing >>>> <https://chromium-review.googlesource.com/c/chromium/src/+/1698345> >>>> Chromium’s >>>> //base double conversion routines with >>>> https://github.com/google/double-conversion. That project is an >>>> extracted version, with a few API tweaks, of the V8 double conversion >>>> routines <https://cs.chromium.org/chromium/src/v8/src/numbers/>. It >>>> turns out that Blink also uses this library >>>> <https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/wtf/dtoa/>, >>>> so I plan on at least unifying Blink on the new copy in //base. >>>> >>> >>> Cool! >>> >> > Thanks :) > > >> >>> >>>> But I have some questions for V8 that could lead to more unification: >>>> >>>> 1) Is the github.com/google/double-conversion project backed by V8, or >>>> is it only loosely affiliated? >>>> >>> >>> Loosely affiliated. I'm not aware of any involvement of current V8 team >>> members with that repository. >>> >>> >>>> 2) Would V8 consider making the external library its source of truth >>>> for double-conversion? >>>> >>> >>> *Consider*, sure, but I can't promise any particular outcome of the >>> consideration ;-) >>> It would certainly be nice not to duplicate maintenance/improvement >>> efforts between the external library and V8's internal version, so I'm >>> generally supportive of consolidating on one source of truth. FWIW, we have >>> at various times toyed with the idea of splitting off various subsystems of >>> V8 as reusable libraries; this would set a nice example of doing that. >>> Concerns I can see include: >>> (1) any kind of change (whether it's a bug fix, a performance >>> improvement, or a refactoring) is easier to do when it doesn't involve >>> multiple repositories. While changes to this code are rare, burdening >>> ourselves with extra difficulty should ideally be outweighed by some >>> benefit (beyond "would be nice"). >>> (2) we need that code to implement JavaScript's semantics, which may or >>> may not be aligned with the goals of the external library. >>> (3) we need that code to compile with GN, whereas the external library >>> seems to only maintain SCons and CMake builds. We could upstream a BUILD.gn >>> file, but see point (1). Considering integration with project-wide build >>> configuration infrastructure, it's likely preferable to keep the BUILD.gn >>> file in V8's repository. >>> (4) we need that code to have test coverage we can trust. I see no CI >>> system on the external library. It does seem to contain a fork of V8's >>> original "cctest" coverage for that code, so presumably we could >>> re-integrate that with our test driver, but it's likely going to be a bit >>> of work. >>> >>> > I agree with all these concerns for V8. > > >> I see that in your CL, you're (for now?) checking a copy of the external >>> github code into the Chromium repository, as opposed to DEPS'ing it in. >>> That addresses some of the concerns, but compromises a bit on the goal of >>> having a single source of truth. In particular, you're running the risk of >>> divergence over time, unless you manually make sure that any change >>> contributed to one copy is also applied to the other. >>> >> > True, it is vendored in as opposed to using DEPS, but I don’t think that > really harms the single source of truth aspect. Chromium vendors in other > third-party libraries and, in my experience, that does not typically end up > leading to many long-standing local modifications. > > >> >>> >>>> 3) Would V8 consider making v8/src/numbers public API, rather than >>>> keeping it in v8::internal, so that Chromium could consume it directly? >>>> >>> >>> I think we would be fine with that. One way to do it would be to >>> introduce a new public header file (strawman: >>> "<v8>/include/v8-double-conversion.h") exposing an API we'll design based >>> on Chrome's (and other embedders'?) needs, and then internally map that >>> onto the various bits in src/numbers/. Alternatively, we could put the code >>> into its own subdirectory (maybe "<v8>/double-conversion/", similar to >>> <chrome>/base/third_party/double-conversion/ in your CL), and make that >>> accessible (through namespace, build system, and documentation) to >>> consumers outside of V8. >>> >> >> This sounds like the ideal outcome for Chromium. V8 would still have some >> of downside 1 you list, but hopefully the double conversion API is fairly >> stable by now. It'd address 3, 4, and it'd enable chrome to ship just one >> copy of this code for chrome, blink, and v8. >> > > I agree that this sounds like the ideal outcome. The one caveat for > Chromium is it would introduce a dependency on > //v8/public:double_conversion or somesuch, but there is some precedent > <https://cs.chromium.org/chromium/src/base/BUILD.gn?l=1619&rcl=414e39a22a5b44fbc0e909c4f50845dcdaf8f422> > for that now. > > >> >> >>> >>> Anyone else have any thoughts? >>> >>> > Assuming no objections to that, I can start experimenting with such an API > after I get Blink converted to the //base copy of double-conversion. I > would probably model it on the API provided by double-conversion (or > Chromium’s and Blink’s usage of it). > > Thanks! > > - Robert > > -- > -- -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAKSzg3QAnK8htSz12Xqprad6C7BPMOQzjawvsS7K6WjTj7TrOQ%40mail.gmail.com.
