-
Notifications
You must be signed in to change notification settings - Fork 293
[ php-wasm ] add intl
extension
#2187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
I tried to optimize the But first of all, these are the sizes of the
My first attempt was to greatly decrease the
it resulted in a 12.7Mb size. Not great, not terrible. The second attempt was to decrease the lib directory with new flags :
Unfortunately PHP won't compile with the So I ended up having my best optimization process :
And here is a comparison between php 8.4 with intl
- export const dependenciesTotalSize = 16143865;
+ export const dependenciesTotalSize = 18472927; php 8.4 with intl -DUCONFIG_NO_LEGACY_CONVERSION=1 -DUCONFIG_NO_COLLATION=1 -DUCONFIGU_NO_FORMATTING=1 -DUCONFIG_NO_TRANSLITERATION=1 -DUCONFIG_NO_REGULAR_EXPRESSIONS=1
- export const dependenciesTotalSize = 16143865;
+ export const dependenciesTotalSize = 18135309; Questions :
|
What are the consequences of having them? Are we missing out on some languages or types of information? Or is it just more compressed? If we retain most information, yes, let's keep those flags on.
Just to summarize the total download size impact for the JSPI build
29MB is way too large for a default download on the web, so let's leave
Would using the latest version be just a matter of changing the build configuration? If so, let's do it. However, if that would create additional compilation hurdles, let's stick with 74.2 for now. It's from December 2023 so still fairly recent.
Thinking about Node.js, a separate file seems fine. Here's a few thoughts I had:
|
Adding the others will disable some php functions like Here are the different sizes without and with php 8.4 without intl
php_8_4.wasm: 16,1 Mb
php_8_4.js: 148 Kb php 8.4 with intl without filters
data : 31,9 Mb
include : 5,1 Mb
lib : 8,5 Mb
php.data: 31,9 Mb
php_8_4.wasm: 18,5 Mb
php_8_4.js: 153 Kb php 8.4 with intl -DUCONFIG_NO_LEGACY_CONVERSION=1 -DUCONFIG_NO_REGULAR_EXPRESSIONS=1 without filters
data : 31,9 Mb
include : 5,1 Mb
lib : 8,2 Mb
php.data: 31,9 Mb
php_8_4.wasm: 18,4 Mb
php_8_4.js: 153 Kb These builds are made with latest ICU version 77.1. Nothing more has to be done to make this version work.
Just to be sure, should I disable
If you add multiple
If I understand that correctly : We have two strategies here. First is the
I am still investigating this but simply instanciating the environment variable
Adding that line in the This is not the |
Only in web, let's still build the Node version with |
Gotcha! What would it take to still rename it, though? Would it be as simple as a string replacement in the built
We'll need to keep Asyncify until Blink (Safari, Bun) supports JSPI 😢
Yes, e.g. XDebug is a dynamic library and it's a short term priority. Lazy loading will be challenging in that we'll need to create extension stubs with the right function signatures to trick PHP into thinking it actually loaded the extension.
I've meant this one: But it seems to be called too late. Hm. There's always the ENV here that we can control without messing with the Perhaps there's some elegant way of injecting that env variable from here: Or maybe baking it into the php.js module is for the best, since it depends on the build options. Looping in @brandonpayton for thoughts |
Yes but there is one for
It is as easy as it looks. What would you like to name it? Maybe
This works as you mentioned :
However, it implies that the path can be changed, while in reality it's fixed at build time based on this line in
But it is indeed way more elegant and it means we can avoid a |
If we are configuring a fixed path that we completely control, it seems like it would be cleanest to just bake a global into the build. I haven't digested all the details in this PR, but adding another |
This is great! Lovely! To confirm my understanding:
Is that right? If yes then yes, let's build all php versions WITH_INTL. Then, separately from this PR, let's discuss the API to load the dat file on the web. In Node we can just always load it. |
@adamziel That's right! I probably need some extra informations :
PHP will still work, and intl extension will be loaded, but when running intl functions without the data from the dat file, php exceptions will be thrown. |
For
I'm confused. I thought it worked as well without
This is fine for v1. For v2, let's explore disabling those functions – I worry some developers might check the availability of the |
@adamziel I was wrong about the size of
Users could add it manually in
Users could add it manually after const php = new PHP( await loadNodeRuntime( '8.3' ) );
php.mkdir( '/icu-data-path' );
php.writeFile( '/icu-data-path/icudt74l.dat', fs.readFileSync( 'node_modules/@php-wasm/node/shared/icudt74l.dat' ) ); OR I should do it in the code, around
export async function loadNodeRuntime(
phpVersion: SupportedPHPVersion,
options: PHPLoaderOptions = {}
) {
const emscriptenOptions: EmscriptenOptions = {...};
const id = await loadPHPRuntime(
await getPHPLoaderModule(phpVersion),
await withNetworking(emscriptenOptions)
);
const php = new PHP( id );
php.mkdir( '/icu-data-path' );
php.writeFile( '/icu-data-path/icudt74l.dat', new Uint8Array( readFileSync( `${__dirname}/shared/icudt74l.dat` ) ) );
return id;
} And yes, this is really bad. But now this code works without having to indicate a ENV variable or loading a data file by myself : import { PHP } from '@php-wasm/universal';
import { loadNodeRuntime } from '@php-wasm/node';
const code = `<?php
$formatter = new \NumberFormatter('en-US', \NumberFormatter::CURRENCY);
var_dump($formatter->format(100.00));
$formatter = new \NumberFormatter('fr-FR', \NumberFormatter::CURRENCY);
var_dump($formatter->format(100.00));`
const php = new PHP( await loadNodeRuntime( '8.3' ) );
const result = await php.run( { code : code } );
console.log( result.text );
But honestly, this is not the right solution. What do you think about it ?
Apologies for the confusion. It works as well without, I just wanted to know what was the best way for you, and it seems to be the "after runtime loaded" way. |
@adamziel Regarding the directories, I suggest creating a
This setup will streamline the transfer of the
I think |
Shared directory sounds great! The rest I'll address on Monday, but the rule of thumb is this: we dont want the minimum download size by more than 2-3 MB |
Motivation for the change, related issues
Mostly based on @oskardydo excellent work and pull request #2173
Implementation details
Refactoring of #2173, several modifications :
gettext
extension separation fromintl
root/lib/data
directory composed oflibicud74l.dat
file only incompile/libintl
andMakefile
buildconf
incompile/php/Dockerfile
--pre-js
flag adds environment variables to PHP during build [ likeICU_DATA
path ]--preload-file
flag will import external files to the build. Useful when files likelibicud74l.dat
are necessary.ZEND_BROKEN_SPRINTF
needs to be disabled in order for PHP 7.1 to build successfully.Questions :
PHP 7.0 currently doesn't support
intl
. Because it needs its own old version of ICU. A version where PHP and ICU were more intimately linked. But I think PHP 7.0 and PHP 7.1 are not supported anymore, right ?The ICU version is
74.2
, version used in base pull request. Should I use a more recent or older release ?The preloaded ICU data file is located in
/internal/shared/preload
. Since this file is mandatory in order for PHP to work properly, should I create another more specific directory ?I didn't find another way to inject
ICU_DATA
inside thephp-wasm
build except with the use of--pre-js
. But instead of adding yet another possibly useless file, do you think of a better way? Maybe that--pre-js
flag and.env.js
file can be useful in the future ?It is currently available for
web
andnode
, but should I wait for theicu
compression tests results or is it definitelynode-only
?Next :
libintl
files, libraries and data to make sure this can be loaded inweb
?P.S. : The first commit is composed of only the files without the builds. For readibility. Next commit will add
libintl
libraries andweb/jspi/7.1
,web/jspi/8.4
,node/asyncify/7.1
,node/asyncify/8.4
.Testing Instructions
Run this with
php.run
orphp.cli
Should return this result :