Note that if you DEPS it into V8, it probably needs to either live on chromium.googlesource.com or mirrored on it.
Yang On Wed, Sep 4, 2019, 14:06 Jakob Kummerow <[email protected]> wrote: > 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 > <https://groups.google.com/d/msgid/v8-dev/CAKSzg3QAnK8htSz12Xqprad6C7BPMOQzjawvsS7K6WjTj7TrOQ%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > -- -- 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/CAFSTc_hmz77vm%3DG6vsTsWMNTvi6gNekXVdEai6H5_sR-XA3O3w%40mail.gmail.com.
