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

Make the PPX emit less sections #387

Merged
merged 6 commits into from Oct 6, 2016

Conversation

Projects
None yet
3 participants
@Drup
Copy link
Member

Drup commented Sep 30, 2016

Apparently, the eliom ppx is a bit too eager to put sections everywhere.
Those two small optimisations are backported from the eliom compiler. This is basically untested.

@vouillon, can you take a look?

Ppx: Avoid sections when they are unnecessary
Namely, for all toplevel statement that do not lead to any evaluation.

@Drup Drup force-pushed the ppx_opt branch from 91e0174 to c0ec077 Sep 30, 2016

@vasilisp

This comment has been minimized.

Copy link
Contributor

vasilisp commented Sep 30, 2016

@pwbs could you test if this patch magically resolves #362 ? A bit far-fetched, I know, but the trouble somehow magically arose by the patterns produced by our PPX, and these patterns will now change.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Sep 30, 2016

@vasilisp I don't think that's far fetched, since that's the reason @vouillon gave me to apply that optim. :)

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 3, 2016

Indeed, it seems to magically resolves #362 :), at least on the os.pgocaml distillery template.

@vasilisp

This comment has been minimized.

Copy link
Contributor

vasilisp commented Oct 3, 2016

Great news!

The code looks good.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 3, 2016

@vouillon I assume you tested on the besport website ? I would prefer some testing before we merge that :)

close_server_section loc ;
]
[ item ] @
(if flush_injection () then

This comment has been minimized.

@vouillon

vouillon Oct 3, 2016

Member

The open_client_section should be removed. Not the close_server_section. Right?

This comment has been minimized.

@Drup

Drup Oct 3, 2016

Author Member

Indeed! I'll fix that in the afternoon.

This comment has been minimized.

@Drup

Drup Oct 3, 2016

Author Member

Should now be fixed.

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 3, 2016

So, the second commit is broken.
And I could not reproduce the crash with the os.pgocaml distillery template, so I can't tell after all whether this fixes the problem...
I'm going to try with besport.

@Drup Drup force-pushed the ppx_opt branch from c0ec077 to 5214c11 Oct 3, 2016

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 3, 2016

So, the crash still happens with the first commit only.
And the second commit is still broken (this can be reproduced with the os.pgocaml distillery template).

@@ -128,7 +138,8 @@ module Pass = struct
(** Syntax extension *)

let client_str item =
if not @@ must_have_section item then [item]
if not @@ must_have_section item
|| flush_injection () then [item]

This comment has been minimized.

@vouillon

vouillon Oct 3, 2016

Member

That should be not (must_have_section item && flush_injection ())

This comment has been minimized.

@Drup

Drup Oct 3, 2016

Author Member

Oups. Fixed.

@Drup Drup force-pushed the ppx_opt branch from 5214c11 to 1f2226e Oct 3, 2016

@Drup Drup force-pushed the ppx_opt branch from 1f2226e to cd8e323 Oct 3, 2016

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 3, 2016

The besport website is still crashing on Safari with this change :(

But I think we are still emitting a lot more close_server_section than with the camlp4 syntax extension.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 3, 2016

The thing is, if you have this piece of code:

{client{
let x = 2
let y = 3 
}}

the camlp4 extension will consider that as only one section. The ppx consider it has two sections. The reason is to handle that kind of cases correctly:

{shared{
let x = 3 
let y = 3 + {client{ ~%x }}
}}

This doesn't work in the camlp4 extension, you must split in two different sections. In the ppx, it'll work directly, since sections are naturally split.

We can probably find a better scheme by analyzing things less locally. I'll try to think about it.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 4, 2016

Several new ideas:

  1. If the declaration doesn't lead to any evaluation (in particular, is a function declaration), we can remove the call to close.
  2. Successive server sections do not need to be separated by a closer_server_section. We can have one close after the last successive server section.
  3. We emit many constant string that are equals. We could share them.

Drup added some commits Oct 5, 2016

Ppx: Use a finer criterium for section insertion.
Now, there are two orthogonal criteria:
- A client section is needed if the declaration contains injections
- A server section is needed if the declaration can lead to execution of
  a fragment (according to the module Cannot_have_fragment).

The core is quite clearer now.
Hoist the hash of the file at the toplevel.
Allow to use string constants only once, which should lead to small
client code.
@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 5, 2016

I implemented idea 1 and 3.
Using ocsigen-start's os.pgocaml template as a benchmark.

close_server_section open_client_section register_client_closure Constant strings
Current 747 1053 217 6303
31bcbd8 665 963 217 6135
cd8e323 665 47 217 5219
6bd4f58 665 47 217 5219
4b12707 278 47 217 4832
7099dfb 278 47 217 4577
288a58c 160 47 217 4546

I can probably reduce it further by fine tuning Cannot_have_fragment. I'm a bit disappointed by the limited impact of idea 3.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 5, 2016

The javascript was compiled with -jsopt --noinline -jsopt --disable=shortvar -jsopt --pretty.

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 5, 2016

Further improvements on idea 1 to eliminate module elements. It looks quite good now. :)

close_server_section open_client_section register_client_closure Constant strings
Current 747 1053 217 6303
288a58c 160 47 217 4546
@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 5, 2016

@vouillon Running grep -o local/var/www/<app>/<app>.js -e '"[^"]*"' on the os.pgocaml example is a bit striking, there is a lot of room for improvement.

In particular, if we run grep -o local/var/www/<app>/<app>.js -e '"[^"]*"' | sort | uniq -c | sort -nr on the compressed version, we can see that there is 259 instances of "number"...

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 5, 2016

(For the curious, as expected, the size impact is quite small, around 4.5% without gziping)

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 5, 2016

The repetition of the string "number" is deliberate. More efficient code is produced for the expression typeof x == "number" than for typeof x == y when y is bound to "number".

@vouillon

This comment has been minimized.

Copy link
Member

vouillon commented Oct 6, 2016

The result is impressive. And Safari no longer crashes!

open_client_section close_server_section
camlp4 3200 2627
ppx(master) 3328 2748
ppx_opt 374 484

There is still room for improvement. For file os_services.eliom in ocsigen-start, I think we only need one close_server_section and one open_client_section, but we get the following code:

close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
close_server_section(eliom_compilation_unit_id_fhytq);
open_client_section(eliom_compilation_unit_id_fhytq);
var main_service$0=get_injection(_akR_,_akQ_,_akP_);
open_client_section(eliom_compilation_unit_id_fhytq);
get_injection(_akU_,_akT_,_akS_);
open_client_section(eliom_compilation_unit_id_fhytq);
get_injection(_akX_,_akW_,_akV_);
open_client_section(eliom_compilation_unit_id_fhytq);
get_injection(_ak0_,_akZ_,_akY_);
open_client_section(eliom_compilation_unit_id_fhytq);
get_injection(_ak3_,_ak2_,_ak1_);
open_client_section(eliom_compilation_unit_id_fhytq);
var connect_service=get_injection(_ak6_,_ak5_,_ak4_);
open_client_section(eliom_compilation_unit_id_fhytq);
var disconnect_service=get_injection(_ak9_,_ak8_,_ak7_);
open_client_section(eliom_compilation_unit_id_fhytq);
var action_link_service=get_injection(_ala_,_ak$_,_ak__);
open_client_section(eliom_compilation_unit_id_fhytq);
var set_password_service=get_injection(_ald_,_alc_,_alb_);
open_client_section(eliom_compilation_unit_id_fhytq);
get_injection(_alg_,_alf_,_ale_);
@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 6, 2016

@vouillon Nice! You can merge after you reviewed/when you feel like it, so that besport can avoid all those forks. :)

I'll do a separate PR if I get more improvements (with idea 2, in particular).

@vouillon vouillon merged commit b1c70d5 into master Oct 6, 2016

1 check passed

default Build finished. No test results found.
Details

@vouillon vouillon deleted the ppx_opt branch Oct 6, 2016

@Drup

This comment has been minimized.

Copy link
Member Author

Drup commented Oct 6, 2016

@vouillon I would be interested by a comparison of the size of request data for besport too, if it's not too complicated to do.

@vasilisp

This comment has been minimized.

Copy link
Contributor

vasilisp commented Oct 6, 2016

Well done @Drup :).

@vasilisp vasilisp referenced this pull request Oct 6, 2016

Closed

Client value trouble #202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment