Make full-icu the default #29522

srl295 · 2019-09-10T18:55:56Z

Fixes: #19214

Contains tooling, and data for ICU 64

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Historical note: yes, this is a reversal of nodejs/node-v0.x-archive#6371 (comment) (7 May 2014) for node v0.12. FYI @tjfontaine and @trevnorris !

srl295 · 2019-09-10T18:57:40Z

please try it out

tools/icu/README.md

gengjiawen · 2019-09-11T03:44:38Z

Also tried this, it works :). cc @mathiasbynens

const f1 = new Intl.NumberFormat('fr');
f1.format(12345678901234567890n);

const formatter = new Intl.NumberFormat('en', {
    style: 'unit',
    unit: 'kilobyte',
  });

const t = formatter.format(1.234);
console.log(t);

gengjiawen · 2019-09-11T03:47:56Z

Should this labeled as semver-major ? Also maybe add Fix #19214 to description or commit message.

srl295 · 2019-09-11T15:36:59Z

linter failure (make lint-py)
fix commit msg

srl295 · 2019-09-13T00:04:37Z

should it really be semver minor or major? It doesn't break anything…

nodejs-github-bot · 2019-09-13T00:06:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25458/

srl295 · 2019-09-13T00:24:41Z

One may ask, "what about full-icu?"

I will tell you.

Installing the npm module full-icu with this already-full node is harmless , and results in:

Note: Your node was not compiled with the ‘small-icu’ case, so the ICU data is not customizable through this script. Exitting.

The install returns successfully because postinstall exits with status 0.

running a full-icu node with node --icu-data-dir=node_modules/full-icu … is also harmless. IF the named directory path list (and it is a list, not a path) contains ICU data or override ICU data, that data is used. If not, it's ignored, because node has enough data to run.

srl295 · 2019-09-13T00:27:35Z

AIX failed,

virtual memory exhausted: Not enough space

I probably need to upgrade the genccode usage.. I'm guessing that configure --with-intl=full-icu --download-all was already broken.

/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/out/Release/obj/gen/icudt64_dat.c

that's a huge file. Maybe ~~100M+~~ 79M of source code (assembly). okay… I can work on improving this here.

BUILDING.md

srl295 · 2019-09-13T17:29:40Z

@nodejs/build-infra is there a way we can get more virtual memory for the AIX build?

srl295 · 2019-09-13T17:59:48Z

Problem: generating a sharedlib or static data (.so/.a/.dll/.lib) from a 25M binary .dat file
This is not a new problem. This situation has been around for 5 years if you build node with --with-intl=full-icu. It's coming to the forefront because we're changing the default.

When I ported the tools, I didn't want to port ICU's dependence on makefiles and generated shell scripts, so I went for a minimal view: always generate an icudt_dat.c file which looks like this:

const struct {
    double bogus;
    uint8_t bytes[27531792];
} icudt64_dat={ 0.0, {
144,0,218,39,20,0,0,0,0,0,2,0,67,109,110,68,
1,0,0,0,3,0,0,0,32,67,111,112,121,114,105,103,
104,116,32,40,67,41,32,50,48,49,54,32,97,110,100,32,…
[zillions of lines omitted]

it's a little harsh on a C compiler. somehow.

Typically how ICU solved this is:

(archaic) we used to generate lots of small separate .c files with data and link them together. That format is deprecated.
on Windows, we have code that can generate a .OBJ file directly.
on many platforms, we generate assembly - an assembler seems to be 'lower overhead' and not use as much memory for such an operation. Even if there are a couple of passes, each pass is more straightforward and uses less temporary memory:

.globl _bar_dat
        .data
        .const
        .balign 16
_bar_dat:
.long 0x27DA0090,0x14,0x020000,0x446E6D43…

The challenge is, there are a lot of assembly formats, even if you arbitrarily and unnecessarily restrict the problem area to only GCC. The assembler is more tied to the OS's loader, etc.

Sooo…

Can we get the memory upped on the builders? that 'kicks the can' down the road, but affects users. As I said, this isn't a new problem.
I can look at using the assembly route— but it probably needs to be a very specific opt-in list for specific environments:

aix/gcc
linuxone/openpower
linux on gcc: If we turn on asm here (will help arm linux platforms, maybe smaller memory footprint etc.)
Did I miss any problematic ones from the build?

Trott · 2019-09-17T18:09:14Z

@nodejs/build-infra is there a way we can get more virtual memory for the AIX build?

/pinging IBMers who are active in Build: @mhdawson @sam-github @gireeshpunathil

sam-github · 2019-09-17T18:51:12Z

I'm in the process of trying to get an AIX7.2 machine working in CI. I think it has twice as much memory. Redoing the AIX6.1 machines, though, I'm not sure how to approach that. I'm asking around.

To temper expectations, more memory or releases/tests being done on AIX7.2 aren't going to happen very fast (sorry, just being honest, some things are slow).

BUILDING.md

srl295 · 2019-09-19T00:13:40Z

OK an update. This seemed to work on mac. PoC. Need to calculate the value of the -a option below from configure.py, and then to add a conditional in gyp to switch in/out the following.

probably "generated-file-suffix" (values: .S, .c, ..?) and '<@(_genccode_opts)' (containing the -a options etc) will work.

Docs on the -a option:

-a or --assembly Create assembly file. (possible values are: gcc, gcc-darwin, gcc-cygwin, gcc-mingw64, sun, sun-x86, xlc, aCC-ia64, aCC-parisc, masm)

so,

linux/gcc -> gcc,
mac -> gcc-darwin,
aix -> xlc? or maybe gcc? not sure.

diff --git a/tools/icu/icu-generic.gyp b/tools/icu/icu-generic.gyp
index b8f0d13836..96930d159c 100644
--- a/tools/icu/icu-generic.gyp
+++ b/tools/icu/icu-generic.gyp
@@ -272,7 +272,7 @@
           'conditions': [
             [ 'icu_small == "false"', {
               # full data - just build the full data file, then we are done.
-              'sources': [ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major)_dat.c' ],
+              'sources': [ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major)_dat.S' ],
               'dependencies': [ 'genccode#host', 'icupkg#host', 'icu_implementation#host', 'icu_uconfig' ],
               'include_dirs': [
                 '<(icu_path)/source/common',
@@ -302,10 +302,11 @@
                 {
                   'action_name': 'icudata',
                   'inputs': [ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major).dat' ],
-                  'outputs':[ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major)_dat.c' ],
+                  'outputs':[ '<(SHARED_INTERMEDIATE_DIR)/icudt<(icu_ver_major)_dat.S' ],
                   'action': [ '<(PRODUCT_DIR)/genccode',
                               '-e', 'icudt<(icu_ver_major)',
                               '-d', '<(SHARED_INTERMEDIATE_DIR)',
+                              '-a', 'gcc-darwin',
                               '-f', 'icudt<(icu_ver_major)_dat',
                               '<@(_inputs)' ],
                 },

nwoltman · 2019-09-22T01:38:46Z

should it really be semver minor or major? It doesn't break anything…

It should be semver major because it can change the result of function calls that rely on ICU. For example, this code:

const fmt = new Intl.NumberFormat('en-CA', {style: 'currency', currency: 'USD'});
console.log(fmt.format(10.32));

currently outputs:

$10.32

but with full-icu, the output will change to:

US$10.32

vsemozhetbyt · 2019-10-08T16:32:00Z

Should our last nightly builds contain full ICU already?

I've tried the last Windows build as of 2019.10.08:

The file size of the node.exe seems unchanged vs 12.11.1 release.
The test from our docs seems failing:

> process.versions
{
  node: '13.0.0-nightly20191008064e111515',
  v8: '7.8.279.14-node.13',
  uv: '1.32.0',
  zlib: '1.2.11',
  brotli: '1.0.7',
  ares: '1.15.0',
  modules: '79',
  nghttp2: '1.39.2',
  napi: '5',
  llhttp: '1.1.4',
  openssl: '1.1.1d',
  cldr: '35.1',
  icu: '64.2',
  tz: '2019a',
  unicode: '12.1'
}
> const january = new Date(9e8);
undefined
> const spanish = new Intl.DateTimeFormat('es', { month: 'long' });
undefined
> spanish.format(january);
'M01'
> spanish.format(january) === 'enero';
false

srl295 · 2019-10-08T16:39:43Z

Should our last nightly builds contain full ICU already?

I've tried the last Windows build as of 2019.10.08:

The file size of the node.exe seems unchanged vs 12.11.1 release.

Seems too small. Can you send/point me to the build log?

richardlau · 2019-10-08T16:54:18Z

I think I can see what the problem is. PR incoming.

targos · 2019-10-08T16:55:40Z

The problem is that the release build is configured with --with-intl=small-icu

richardlau · 2019-10-08T17:04:00Z

PR: #29887

srl295 added wip Issues and PRs that are still a work in progress. i18n-api Issues and PRs related to the i18n implementation. labels Sep 10, 2019

srl295 self-assigned this Sep 10, 2019

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 10, 2019

srl295 requested a review from devsnek September 10, 2019 23:19

gengjiawen reviewed Sep 11, 2019

View reviewed changes

tools/icu/README.md Outdated Show resolved Hide resolved

devsnek mentioned this pull request Sep 11, 2019

Intl.RelativeTimeFormat and Intl.ListFormat don't work on languages other than English on node 12.10.0 #29525

Closed

srl295 removed the wip Issues and PRs that are still a work in progress. label Sep 13, 2019

srl295 force-pushed the full-icu-default branch from 0a471af to 7725d01 Compare September 13, 2019 00:04

srl295 marked this pull request as ready for review September 13, 2019 00:04

srl295 changed the title ~~Full icu default WIP~~ Make full-icu the default Sep 13, 2019

richardlau reviewed Sep 13, 2019

View reviewed changes

BUILDING.md Outdated Show resolved Hide resolved

BUILDING.md Outdated Show resolved Hide resolved

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:50

jasnell reviewed Sep 17, 2019

View reviewed changes

BUILDING.md Outdated Show resolved Hide resolved

Trott closed this Oct 3, 2019

srl295 deleted the full-icu-default branch October 3, 2019 23:12

srl295 mentioned this pull request Oct 4, 2019

ICU 65.1 #29540

Closed

richardlau mentioned this pull request Oct 8, 2019

build: make full-icu the default for releases #29887

Closed

2 tasks

BethGriggs mentioned this pull request Oct 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

This was referenced Oct 9, 2019

What could happend, If you update ICU in Node nodejs/Release#483

Closed

Pull from icu4c github release instead of package nodejs/full-icu-npm#36

Closed

sam-github mentioned this pull request Oct 10, 2019

test-osuosl-aix61-ppc64_be-2 lacks disk space to compile obj/gen/icudt64_dat.S nodejs/build#1951

Closed

richardlau mentioned this pull request Oct 16, 2019

citgm builds with small-icu nodejs/build#1963

Closed

richardlau mentioned this pull request Nov 7, 2019

icu build: use genccode -o on non-windows platforms #21695

Closed

brendankenny mentioned this pull request Nov 17, 2019

Internationalization of Lighthouse (Phase 2) GoogleChrome/lighthouse#7238

Closed

36 tasks

bnoordhuis mentioned this pull request Nov 22, 2019

Strategic initiative to move to cmake for building Node.js? nodejs/TSC#648

Closed

richardlau mentioned this pull request Jan 21, 2020

[v12.x backport] tools: update icu to 65.1 #31433

Closed

Swaagie mentioned this pull request Feb 21, 2020

[patch] use icu data from prebuild binary jcrugzz/s6-node-alpine#1

Merged

bzoz mentioned this pull request Mar 12, 2020

Building LTS v12.x on Windows Fails The system cannot find the file specified." #32044

Closed

srl295 mentioned this pull request Mar 19, 2020

Workaround to prevent ICU trouble #30544

Closed

srl295 mentioned this pull request Nov 10, 2020

doc: advices to use full-icu package is outdated #36057

Closed

1 task

addaleax mentioned this pull request Dec 8, 2020

Why Node.exe doubled .exe filesize? v12 vs v14 nodejs/help#3114

Closed

Uh oh!

Make full-icu the default #29522

Make full-icu the default #29522

Uh oh!

Conversation

srl295 commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

srl295 commented Sep 10, 2019

Uh oh!

Uh oh!

gengjiawen commented Sep 11, 2019

Uh oh!

gengjiawen commented Sep 11, 2019

Uh oh!

srl295 commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Sep 13, 2019

Uh oh!

nodejs-github-bot commented Sep 13, 2019

Uh oh!

srl295 commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

srl295 commented Sep 13, 2019

Uh oh!

srl295 commented Sep 13, 2019

Uh oh!

Trott commented Sep 17, 2019

Uh oh!

sam-github commented Sep 17, 2019

Uh oh!

Uh oh!

srl295 commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nwoltman commented Sep 22, 2019

Uh oh!

vsemozhetbyt commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srl295 commented Oct 8, 2019

Uh oh!

richardlau commented Oct 8, 2019

Uh oh!

targos commented Oct 8, 2019

Uh oh!

richardlau commented Oct 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

srl295 commented Sep 10, 2019 •

edited

Loading

srl295 commented Sep 11, 2019 •

edited

Loading

srl295 commented Sep 13, 2019 •

edited

Loading

srl295 commented Sep 13, 2019 •

edited

Loading

srl295 commented Sep 19, 2019 •

edited

Loading

vsemozhetbyt commented Oct 8, 2019 •

edited

Loading