Bug 438 - add caching/memoization to soc.simulator.program.Program
Summary: add caching/memoization to soc.simulator.program.Program
Status: CONFIRMED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: Source Code (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- major
Assignee: Jacob Lifshay
URL:
Depends on:
Blocks: 324
  Show dependency treegraph
 
Reported: 2020-07-24 04:43 BST by Jacob Lifshay
Modified: 2020-07-24 21:54 BST (History)
3 users (show)

See Also:
NLnet milestone: ---
total budget (EUR) for completion of task and all subtasks: 0
budget (EUR) for this task, excluding subtasks' budget: 0
parent task for budget allocation:
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Lifshay 2020-07-24 04:43:02 BST
because of how the code is currently laid out, Program needs to be created for every test case. Program as currently implemented is quite slow, since it needs to create files on disk and run external programs. The API should be changed to cache compiled code in memory, try looking up the instructions to compile in a cache before mucking about on disk.
Importantly: it needs to *NOT* keep a file open for every Program instance.

My random guess is it could speed up div pipe testing by a factor of 2 in the single-threaded case (more if you don't have a SSD), and easily many more in the multi-threaded case.
Comment 1 Luke Kenneth Casson Leighton 2020-07-24 10:47:16 BST
(In reply to Jacob Lifshay from comment #0)
> because of how the code is currently laid out, Program needs to be created
> for every test case. Program as currently implemented is quite slow, since
> it needs to create files on disk and run external programs. The API should
> be changed to cache compiled code in memory, try looking up the instructions
> to compile in a cache before mucking about on disk.
> Importantly: it needs to *NOT* keep a file open for every Program instance.
> 
> My random guess is it could speed up div pipe testing by a factor of 2 in
> the single-threaded case (more if you don't have a SSD), and easily many
> more in the multi-threaded case.

it can't be run multi-threaded: i found that out when trying to put
the construction of the Simulator object into unit test setup/teardown.

the reason is that once a nmigen Simulator has been used in a yield loop,
it may not be re-used.  not even calling "reset" will work.

also: running in parallel will actually interfere with the ordering in
which the tests are created (and appended to test_data).

that said: if you feel that the amount of time saved (by all of us)
would be *significantly* greater than the time taken to create a compile
cache please do go ahead.
Comment 2 Luke Kenneth Casson Leighton 2020-07-24 11:56:36 BST
commit 75858e9e95e25f882fa130f50778f18fe88aa742 (HEAD -> master)
Author: Luke Kenneth Casson Leighton <lkcl@lkcl.net>
Date:   Fri Jul 24 11:54:24 2020 +0100

    read into a BytesIO to avoid "too many open files"
     see https://bugs.libre-soc.org/show_bug.cgi?id=438

should do for now

arg nope, linking still leaves files open.  increasing the file limit
for now

/etc/security/limits.conf



  File "fu/div/test/test_pipe_caller.py", line 211, in test_all
    prog = Program(l, bigendian)
  File "/home/lkcl/src/libresoc/soc/src/soc/simulator/program.py", line 39, in __init__
    self._assemble()
  File "/home/lkcl/src/libresoc/soc/src/soc/simulator/program.py", line 80, in _assemble
    self._link(outfile)

    restore_signals, start_new_session)
  File "/usr/lib/python3.7/subprocess.py", line 1412, in _execute_child
    errpipe_read, errpipe_write = os.pipe()
OSError: [Errno 24] Too many open files
Comment 3 Luke Kenneth Casson Leighton 2020-07-24 12:04:01 BST
ulimit -n 10240

aw fer god's sake it requires a frickin reboot
https://underyx.me/articles/raising-the-maximum-number-of-file-descriptors
Comment 4 Luke Kenneth Casson Leighton 2020-07-24 12:06:30 BST
got it.

diff --git a/src/soc/fu/div/test/test_pipe_caller.py b/src/soc/fu/div/test/test_pipe_caller.py
index 6753f8ad..a5e83887 100644
--- a/src/soc/fu/div/test/test_pipe_caller.py
+++ b/src/soc/fu/div/test/test_pipe_caller.py
@@ -208,8 +208,9 @@ class DivTestCases:
                     initial_regs = [0] * 32
                     initial_regs[1] = ra
                     initial_regs[2] = rb
-                    prog = Program(l, bigendian)
-                    self.run_test_program(prog, initial_regs)
+                    # use "with" so as to close the files used
+                    with Program(l, bigendian) as prog:
+                        self.run_test_program(prog, initial_regs)
Comment 5 Luke Kenneth Casson Leighton 2020-07-24 13:07:59 BST
(In reply to Jacob Lifshay from comment #0)

> Importantly: it needs to *NOT* keep a file open for every Program instance.

this is sorted by using the "with" pattern.  the "with" pattern calls
__enter__ and __exit__.  in Program.__exit__ self.close() is called.

because this had been used:

     x = Program()
     x.add_test()

the close function was not being called.  it should have been:

     x = Program()
     x.add_test()
     x.close()

however the whole purpose of "with" is to avoid that extra line and
avoid polluting code with entry/exit gunk.

hence the change to:

      with Program() as x:
         x.add_test()

and the problem is now gone.
Comment 6 Jacob Lifshay 2020-07-24 21:15:32 BST
does anything depend on Program referring to a file and not a memory buffer? if not, I'll just move the file stuff into the constructor and add caching. this should reduce the test setup time from 2min to a few seconds for the sim-only div tests. It will also greatly reduce wear on my SSD, since it doesn't need to create 100k files.
Comment 7 Jacob Lifshay 2020-07-24 21:20:53 BST
(In reply to Luke Kenneth Casson Leighton from comment #1)
> (In reply to Jacob Lifshay from comment #0)
> > because of how the code is currently laid out, Program needs to be created
> > for every test case. Program as currently implemented is quite slow, since
> > it needs to create files on disk and run external programs. The API should
> > be changed to cache compiled code in memory, try looking up the instructions
> > to compile in a cache before mucking about on disk.
> > Importantly: it needs to *NOT* keep a file open for every Program instance.
> > 
> > My random guess is it could speed up div pipe testing by a factor of 2 in
> > the single-threaded case (more if you don't have a SSD), and easily many
> > more in the multi-threaded case.
> 
> it can't be run multi-threaded: i found that out when trying to put
> the construction of the Simulator object into unit test setup/teardown.

I meant multi-process, though I wasn't planning on sharing a Simulator between tests at all, though each test still tests everything in test_data.

> the reason is that once a nmigen Simulator has been used in a yield loop,
> it may not be re-used.  not even calling "reset" will work.
> 
> also: running in parallel will actually interfere with the ordering in
> which the tests are created (and appended to test_data).

not anymore, I rewrote that class to not use unittest since that was a misuse of unittest. It is now just a class for creating test_data.
Comment 8 Luke Kenneth Casson Leighton 2020-07-24 21:54:26 BST
> > also: running in parallel will actually interfere with the ordering in
> > which the tests are created (and appended to test_data).
> 
> not anymore, I rewrote that class to not use unittest since that was a
> misuse of unittest. It is now just a class for creating test_data.

ah great.

remember that there are 15 other unit tests counting on the same base infrastructure.  fu/*/test_pipe_caller.py and fu/compunit/*.py

(In reply to Jacob Lifshay from comment #6)
> does anything depend on Program referring to a file and not a memory buffer?

not at all although the actual files obviously are created on disk as they need  binutils as and ld to create them.

we are not planning to do hundred gigabyye compiles.  in fact i cannot see going beyond even 50 instructions let alone 1000 instructions for unit tests, given that they are all wtitten in assembler.

> if not, I'll just move the file stuff into the constructor and add caching.
> this should reduce the test setup time from 2min to a few seconds for the
> sim-only div tests. It will also greatly reduce wear on my SSD, since it
> doesn't need to create 100k files.

yehyeh.