ton-blockchain / ton

Main TON monorepo
Other
3.1k stars 984 forks source link

Inline functions are emitted into methods dictionary #226

Closed Skydev0h closed 4 years ago

Skydev0h commented 4 years ago

Inline functions are emitted into methods dictionary despite the fact that they are not ever called from that dictionary.

An example that demonstrates what I mean:

inltest.fc

int imp(int v) impure { return v; } ;; 1
int test_inline(int a, int b) inline_ref { return a + b + 0x11223344; } ;; 2

() recv_internal() impure { int c = test_inline(0xAA, 0xBB); imp(c); } ;; 0
() recv_external() impure { int c = test_inline(0xCC, 0xDD); imp(c); } ;; -1

inltest.fif

// automatically generated from `inltest.fc`
DECLPROC imp
DECLPROC test_inline
DECLPROC recv_internal
DECLPROC recv_external
imp PROC:<{
}>
test_inline PROC:<{
  ADD
  287454020 PUSHINT
  ADD
}>
recv_internal PROC:<{
  170 PUSHINT
  187 PUSHINT
  test_inline INLINECALLDICT
  imp CALLDICT
  DROP
}>
recv_external PROC:<{
  204 PUSHINT
  221 PUSHINT
  test_inline INLINECALLDICT
  imp CALLDICT
  DROP
}>

inlassess.fif

#!/usr/bin/fift -s

"TonUtil.fif" include
"Asm.fif" include

PROGRAM{ "inltest.fif" include }END>c =: Code

Code <s csr. cr
2 Code <s ref@ 19 idict@ drop .s csr.

result

x{FF00F4A413F4A0F2C80B}
 x{2_}
  x{D0}
   x{2_}
    x{20402AA0402EE8208404488CD1283C004C2_}
    x{2_}
   x{4A0821011223344A0}
  x{F28100CC8100DDA0821011223344A0F00130}

CS{Cell{00114a0821011223344a08} bits: 4..68; refs: 0..0}
x{A0821011223344A0}

At least several cells are wasted and tree complexity is unneccessarily increased. This is especially important if many inline functions are used (i spotted tens of those in some contest 1 and contest 2 entries).

As for inline_ref there is following result:

x{FF00F4A413F4A0F2C80B}
 x{2_}
  x{D0}
   x{2_}
    x{20402AA0402EF6CF3C004C2_}
     x{A0821011223344A0}
    x{2_}
   x{4}
    x{A0821011223344A0}
  x{F28100CC8100DDDB3CF00130}
   x{A0821011223344A0}

CS{Cell{010148} bits: 4..4; refs: 0..1}
x{}
 x{A0821011223344A0}

That way the function cell itself is not completely wasted (because it is referenced later as-is when called, but still, several cells are wasted as part of dictionary itself and complexity is still unneccessarily increased.

Therefore, it should be safe to remove (do not put into) those functions from procs-methods dict because they are not referenced directly by id in code (they are body-inlined or ref-inlined). It may be neccessary only if for some reason function cannot be inlined (such as recursive calls).

And of course it would be wise not to touch methods with defined id, only procs (with autogenerated id), however it would be strange pattern to have inline methods.

More broadly speaking, fift assembler can keep track of used procs (that are called by dict id) plus root functions (0, -1, -2, -3) and getter methods and throw out the rest.

Skydev0h commented 4 years ago

Ref: #230 (implementation / fix)

ton-blockchain commented 4 years ago

(Almost) fixed in commit 493ae2410cda47dffc7e793d5db28fa84c4f88a5, where unused non-external functions are also often removed from dictionary.

But if function f() calls g(), and both f() and g() are not called from anywhere else, then only f() will be removed from the dictionary.

Skydev0h commented 4 years ago

Yeah, good point, now I understand that my suggestion would suffer from the same problem. Tracking down reference counts of each call and analyzing body upon exclusion would be really difficult and bug-prone. It may be possible to track invocations of each method in separate dict and manipulate counters from there.

I hope I would come out with a good idea and implementation sometime.

ton-blockchain commented 4 years ago

Well, the current solution (as well as the solution suggested by you) solves the original problem - correctly removes all inline procedures from the dictionary. Tracking the graph of dependencies between all functions is better done by the FunC compiler.

Skydev0h commented 4 years ago

Yeah, well, good idea. Sounds like a challenge for ... sometime. I agree that the original technical problem is now solved.