Skip to content
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

symtab: use longjmp() for errors and avoid intermediate table. #330

Merged
merged 3 commits into from
Nov 1, 2020

Conversation

haberman
Copy link
Member

@haberman haberman commented Nov 1, 2020

We used to use a separate "add table" during the upb_symtab_addfile()
operation to make it easier to back out the file if it contained
errors. But this created unnecessary work of re-adding the same symbols
to the main symtab once everything was validated.

Instead we directly add symbols to the main symbols table. If there is
an error in validation, we remove precisely the set of symbols that
were already added.

This also requires using a separate arena for each file. We can fuse
it with the symtab's main arena if the operation is successful.

This gives a nice boost to the Ads descriptor loading benchmark:

LoadDescriptor_Upb                   61.2µs ± 4%  53.5µs ± 1%  -12.50%  (p=0.000 n=12+12)
LoadAdsDescriptor_Upb                4.43ms ± 1%  3.06ms ± 0%  -31.00%  (p=0.000 n=12+12)
LoadDescriptor_Proto2                 257µs ± 0%   259µs ± 0%   +1.00%  (p=0.000 n=12+12)
LoadAdsDescriptor_Proto2             13.9ms ± 1%  13.9ms ± 1%     ~     (p=0.128 n=12+12)

Unfortunately switching to longjmp() for error handling seems to grow
the code a bit:

    FILE SIZE        VM SIZE                 
 --------------  --------------                                                                                             
  +225% +2.74Ki  [ = ]       0    [Unmapped]                                                                                
  +7.1% +1.16Ki  +7.4% +1.05Ki    upb/def.c                                                                                 
     +17%    +592   +17%    +592    _upb_symtab_addfile                                                                     
    [NEW]    +276  [NEW]    +240    check_ident
     +24%    +272   +25%    +272    resolve_fielddef
    [NEW]    +236  [NEW]    +200    symtab_errf
    +7.8%    +224  +7.9%    +224    create_msgdef
    [NEW]    +146  [NEW]    +104    upb_oneofdef_name         
    [NEW]    +126  [NEW]     +88    symtab_oomerr   
    [NEW]     +90  [NEW]     +48    upb_status_setoom
    -1.8%     -16  -1.8%     -16    create_enumdef
    -2.6%     -48  -2.6%     -48    create_fielddef                                                                         
   -17.0%     -48 -19.4%     -48    symtab_add                                                                              
    [DEL]    -260  [DEL]    -224    upb_isident
    [DEL]    -399  [DEL]    -360    symtab_resolve
  +5.3%    +136  +6.7%    +128    upb/upb.c
    [NEW]    +215  [NEW]    +176    upb_arena_fuse                                                                          
    [DEL]     -79  [DEL]     -48    upb_ok                    
  +2.2%     +96  +2.2%     +96    [section .rodata]
  +0.2%     +13  [ = ]       0    upb/table.c
     +12%     +13  [ = ]       0    upb_strtable_iter_key                                                                   
  -1.0%     -13  [ = ]       0    upb/msg.c                   
    -4.1%     -13  [ = ]       0    _upb_array_realloc
  +2.6% +4.12Ki  +1.0% +1.27Ki    TOTAL

We used to use a separate "add table" during the upb_symtab_addfile()
operation to make it easier to back out the file if it contained
errors. But this created unnecessary work of re-adding the same symbols
to the main symtab once everything was validated.

Instead we directly add symbols to the main symbols table. If there is
an error in validation, we remove precisely the set of symbols that
were already added.

This also requires using a separate arena for each file. We can fuse
it with the symtab's main arena if the operation is successful.

LoadDescriptor_Upb                   61.2µs ± 4%  53.5µs ± 1%  -12.50%  (p=0.000 n=12+12)
LoadAdsDescriptor_Upb                4.43ms ± 1%  3.06ms ± 0%  -31.00%  (p=0.000 n=12+12)
LoadDescriptor_Proto2                 257µs ± 0%   259µs ± 0%   +1.00%  (p=0.000 n=12+12)
LoadAdsDescriptor_Proto2             13.9ms ± 1%  13.9ms ± 1%     ~     (p=0.128 n=12+12)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants