#10296 closed enhancement (fixed)
Singular interface wasting time by waiting for the prompt too often
Reported by: | SimonKing | Owned by: | was |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | interfaces | Keywords: | Singular, _eval_line, synchronization, synchronisation |
Cc: | was, wjp, AlexanderDreyer, malb, leif | Merged in: | sage-5.0.beta8 |
Authors: | Simon King | Reviewers: | Martin Albrecht |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11431 | Stopgaps: |
Description (last modified by )
There are two fundamental differences between the Singular and the Gap interfaces:
- The Singular interface uses a synchronisation method in each call to its eval method. Gap doesn't.
- The Gap interface deletes variables by declaring that their name may be overwritten. This is not possible in Singular, since Singular variables are statically typed. Thus, the Singular interface explicitly kills the underlying variable in Singular, which requires one call to _eval_line for each variable that is to be deleted.
Unfortunately, waiting for the prompt to appear in a pseudo terminal may be very costly on some systems, since select.select() may be very slow - see #10294. So, additional calls to _eval_line should be strictly avoided!
The first point makes me wonder: Why has the synchronisation method become necessary for Singular but not for Gap?
The second point makes me wonder whether it is really necessary to use one _eval_line for each single deletion.
Moreover, I wonder whether the two points are actually related: I could imagine that synchronisation became necessary when garbage collection, accidentally performed inside an _eval_line, and thus sending an _eval_line inside an outer _eval_line, caused a dead lock in the outer _eval_line. Was that the case, historically?
Suggestions:
singular.eval(cmd)
is currently simply passingcmd
to _eval_line, after synchronisation and after killing unused variables. I suggest to add the code for killing unused variables tocmd
, thus using _eval_line only once.- If my above guess on the role of synchronisation is correct, then one could simply drop synchronisation after implementing my first suggestion.
This is related with #10294 and perhaps with #10295. Cc to William and Willem-Jan, since they wrote the synchronisation code.
Depends on #7377.
For the release manager:
Apply
Attachments (4)
Change History (74)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
- Cc AlexanderDreyer added
comment:3 Changed 8 years ago by
- Status changed from new to needs_review
- Summary changed from Singular interface wasting time by calling select.select() too often to Singular interface wasting time by waiting for the prompt too often
My patch does the following.
Garbage Collection
In the Singular interface, freeing the memory used for old variables in Singular requires to send a "kill" command to Singular, presumably by _eval_line.
A problem would result if garbage collection would create such _eval_line inside an outer _eval_line: The inner _eval_line would cause the outer _eval_line to hang forever. I guess this was the reason why the del method does not actually delete the variables but only marks them for deletion; the actual deletion is postponed to the next use of "eval".
I guess that for the same reason, synchronisation is done at the beginning of "eval". Please correct me, if the reason for synchronising so frequently is different'''
I suggest to do the deletions a bit less frequently, namely in the "set" method rather than in the "eval" method. Moreover, I suggest to send the kill command not by a separate _eval_line (one for each old variable), but to prepend the join of all kill commands to the command that creates the new variable.
Since killing does not require an additional _eval_line, nesting can not occur. So, synchronisation is not needed.
Overhead Reduction
As mentioned in the ticket description, calling _eval_line frequently is bad, since waiting for the Singular prompt in the output of a pseudo terminal produces a massive overhead (up to 22ms per call) on some machines.
My patch removes synchronisation (which avoids one call to _eval_line per call to eval), and killing old variables does not require any additional _eval_line. So, in the example proposed in the ticket description, the wall time drops by 2/3!
Miscellaneous
Synchronisation used to fail with an AttributeError
for the GAP interface. It is fixed by the patch.
I tried to make everything more stable, by giving more opportunities to detect that the interface crashed, and restarting if necessary.
Of course, I added documentation and tests that (I think) cover all issues mentioned here. Moreover, I added tests covering some optional arguments to _eval_line.
For me, sage -testall
passes. Hence, ready for review!
comment:4 Changed 8 years ago by
The old patch needed to be rebased.
The new patch version should apply on top of sage-4.6.1.alpha3
. I am now running doctests, but I'll keep it "needs review" for now.
comment:5 follow-up: ↓ 6 Changed 8 years ago by
- Status changed from needs_review to needs_work
Not good. There are several timeouts in the tests. I need to work on it.
comment:6 in reply to: ↑ 5 Changed 8 years ago by
- Status changed from needs_work to needs_review
Replying to SimonKing:
Not good. There are several timeouts in the tests.
It seems that I forgot sage -b
. I repeated the tests that previously had a timeout, and now it is fine.
I put it back to "needs review".
comment:7 Changed 8 years ago by
Since the nagbot currently does not seem to send messages: I hope you don't mind that I'm sending a reminder about speeding up the Singular pexpect interface.
comment:8 Changed 8 years ago by
I am sorry to nag you again - could someone take a look at the patch? The patchbot has no complaints at that point.
comment:9 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to fix tests
Too late...
Meanwhile the patchbot finds some errors. I try to find the reason.
comment:10 Changed 8 years ago by
- Status changed from needs_work to needs_review
- Work issues fix tests deleted
Apparently, a StdOutContext
was recently introduced, and the expected output for one doctest has changed by my patch. I changed the test slightly, so that now it should work.
comment:11 Changed 8 years ago by
- Cc malb added
I think reducing the overhead in the Singular pexpect interface by 2/3 is a good thing. But the ticket is starting to become old.
Review, anyone?
comment:12 Changed 8 years ago by
- Reviewers set to Martin Albrecht
- Status changed from needs_review to positive_review
- Type changed from defect to enhancement
- the patch applies cleanly against 4.7.alpha3
- long doctests pass
- the patch is well documented
- the patch does look okay
So all good. The only caveat: I cannot claim I thought long and hard about the possible implications of changing this stuff. But unless someone comes up with a good example why Simon's approach is wrong, I'd say positive review.
comment:13 follow-up: ↓ 14 Changed 8 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
- Status changed from positive_review to needs_work
This will need to be rebased because it conflicts with #7377.
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 15 Changed 8 years ago by
comment:15 in reply to: ↑ 14 Changed 8 years ago by
Replying to SimonKing:
Replying to jdemeyer:
This will need to be rebased because it conflicts with #7377.
Really unfortunate. Apparently, in order to apply #7377 to my sage-4.6.2, I also need to import further patches.
The easiest solution is probably to download http://boxen.math.washington.edu/home/release/sage-4.7.alpha5/sage-4.7.alpha5.tar which includes #7377. This version of Sage is not released and will certainly change and might have doctest failures but at least it gives you a target to rebase to.
comment:16 follow-up: ↓ 17 Changed 8 years ago by
- Work issues set to make gap restart after quitting or crashing
It is really really unfortunate. The changes to the gap interface (so that gap restarts after quitting, instead of throwing an error) are now not working any more.
comment:17 in reply to: ↑ 16 ; follow-ups: ↓ 18 ↓ 20 Changed 8 years ago by
- Status changed from needs_work to needs_review
- Work issues make gap restart after quitting or crashing deleted
I updated my patch. It should now apply to sage-4.7.alpha5. The long tests in sage/interfaces/
all pass.
Replying to SimonKing:
It is really really unfortunate. The changes to the gap interface (so that gap restarts after quitting, instead of throwing an error) are now not working any more.
The reason for that was a typo when I applied one of the hunks manually.
Note that I changed .interrupt()
into .interrupt(timeout=1)
in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.
Does that suffice to return to the positive review?
In any case, I am now running all long doctests.
comment:18 in reply to: ↑ 17 Changed 8 years ago by
Replying to SimonKing:
Note that I changed
.interrupt()
into.interrupt(timeout=1)
in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.Does that suffice to return to the positive review?
In any case, I am now running all long doctests.
FWIW: The long doctests all pass.
comment:19 Changed 8 years ago by
- Status changed from needs_review to positive_review
yes, back to positive review.
comment:20 in reply to: ↑ 17 ; follow-up: ↓ 21 Changed 8 years ago by
Replying to SimonKing:
Note that I changed
.interrupt()
into.interrupt(timeout=1)
in two doctests. I don't know why, but they failed in about half of the cases. With the timeout parameter, they pass in 10 out of 10 runs.
I haven't looked at the patch in detail, but unexplained behaviour related to synchronization/interrupts in a patch modifying synchronization sounds a bit scary, and a reason to think about it some more...
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 24 Changed 8 years ago by
Replying to wjp:
I haven't looked at the patch in detail, but unexplained behaviour related to synchronization/interrupts in a patch modifying synchronization sounds a bit scary, and a reason to think about it some more...
For the record: The test in question is new.
In sage-4.6 (as I just tested on sage.math), the test would in fact not work at all: When you do
sage: cutoff = singular._eval_using_file_cutoff sage: singular._eval_using_file_cutoff = 4 sage: singular._eval_line('for(int i=1;i<=3;i++){i=1;};', wait_for_prompt=False)
then you needed to interrupt it manually.
Now, the problem is that after the lines above, sage: singular.interrupt()
is supposed to return False
, which it always did on the command line, but in some (not all) doctest runs returns True
.
Since we wouldn't have been able to test it without my patch, we actually don't know
(1) whether the instability existed before and was uncovered by the patch, (2) whether the problem was introduced by my patch, or (3) whether the problem was a result of the changes to the pexpect interface in #7377.
The latter could actually be the case: Today was the first time ever that the singular.interrupt()
example was problematic - and it was the first time that I worked with the patches from #7377.
comment:22 Changed 8 years ago by
- Merged in set to sage-4.7.1.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:23 Changed 8 years ago by
- Description modified (diff)
comment:24 in reply to: ↑ 21 Changed 8 years ago by
Replying to SimonKing:
The latter could actually be the case: Today was the first time ever that the
singular.interrupt()
example was problematic - and it was the first time that I worked with the patches from #7377.
I have decided not to merge #7377 in the sage-4.7 cycle. I will merge it in sage-4.7.1.alpha0, such that potential issues with #7377 can be sorted out.
comment:25 follow-up: ↓ 26 Changed 8 years ago by
Shouldn't this ticket then re-opened as well?
And should I perhaps provide two patch versions here, one depending on #7377 and one independent?
comment:26 in reply to: ↑ 25 Changed 8 years ago by
Replying to SimonKing:
Shouldn't this ticket then re-opened as well?
And should I perhaps provide two patch versions here, one depending on #7377 and one independent?
For the moment, the plan is still to merge #7377 in sage-4.7.1.alpha0 together with #10296 and a fixed #10818. So for the moment, I think we can leave this ticket as it is.
comment:27 follow-up: ↓ 28 Changed 8 years ago by
I've ran more than 100 tests of expect.py and gap.py without "timeout=1" (so using the default value of timeout=0.3) on Sage 4.7.alpha3 + #7377 + #10818 + #10296, built from scratch on debian unstable/experimental amd64, intel core 2 quad, and everything passed.
By the way, hg failed to apply the patch, I had to manually apply the following hunk:
--- gap.py +++ gap.py @@ -475,46 +532,45 @@
The only difference is that the tests are obviously longer with a greater value of timeout.
Could anyone else check whether the default value of timeout gives a random behavior, at least as often as every two times?
comment:28 in reply to: ↑ 27 Changed 8 years ago by
Replying to jpflori:
I've ran more than 100 tests of expect.py and gap.py without "timeout=1" (so using the default value of timeout=0.3) on Sage 4.7.alpha3 + #7377 + #10818 + #10296, built from scratch on debian unstable/experimental amd64, intel core 2 quad, and everything passed.
Quite a relief...
I have Debian GNU/Linux squeeze/sid, and I think our administrator did patch the kernel in order to make pseudo-Tty faster. The reason is that, without that kernel patch, the overhead of all pexpect interfaces is abundant. It is, of course, imaginable that the kernel patch is responsible for the flaky behaviour of the test on my machine.
comment:29 Changed 8 years ago by
Ok, I've now done 500 tests of both without problems.
comment:30 Changed 8 years ago by
- Merged in sage-4.7.1.alpha0 deleted
- Resolution fixed deleted
- Status changed from closed to new
I get many failures on the buildbot, mainly on non-Linux systems (e.g. OS X, Solaris). The failures look like the following:
sage -t -long -force_lib devel/sage/sage/interfaces/expect.py ********************************************************************** File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/interfaces/expect.py", line 702: sage: singular._eval_line_using_file('def a=3;') Exception raised: Traceback (most recent call last): File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_15[5]>", line 1, in <module> singular._eval_line_using_file('def a=3;')###line 702: sage: singular._eval_line_using_file('def a=3;') File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 731, in _eval_line_using_file s = self._eval_line(self._read_in_file_command(tmp_to_use), allow_use_file=False) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 841, in _eval_line raise RuntimeError, "%s\nError evaluating %s in %s"%(msg, line, self) RuntimeError: [Errno 5] Input/output error Error evaluating < "/tmp/dot_sage.gjyZgjfltL/temp/bsd.local/8324//interface//tmp8502"; in Singular ********************************************************************** [..many more similar errors..] sage -t -long -force_lib devel/sage/sage/interfaces/gap.py ********************************************************************** File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/devel/sage-main/sage/interfaces/gap.py", line 521: sage: a = gap(3) Exception raised: Traceback (most recent call last): File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_9[10]>", line 1, in <module> a = gap(Integer(3))###line 521: sage: a = gap(3) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 201, in __call__ return self._coerce_from_special_method(x) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 227, in _coerce_from_special_method return (x.__getattribute__(s))(self) File "sage_object.pyx", line 463, in sage.structure.sage_object.SageObject._gap_ (sage/structure/sage_object.c:3988) File "sage_object.pyx", line 439, in sage.structure.sage_object.SageObject._interface_ (sage/structure/sage_object.c:3628) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/interface.py", line 199, in __call__ return cls(self, x, name=name) File "/Users/buildbot/build/sage/bsd-1/bsd_64_full/build/sage-4.7.1.alpha0/local/lib/python/site-packages/sage/interfaces/expect.py", line 1286, in __init__ raise TypeError, x TypeError: Error evaluating $sage9:=3;; in Gap **********************************************************************
comment:31 follow-up: ↓ 32 Changed 8 years ago by
That's bad. Perhaps its a difference in new line / carriage return characters?
Probably I need to build sage on bsd.math before being able to test.
comment:32 in reply to: ↑ 31 ; follow-ups: ↓ 33 ↓ 34 Changed 8 years ago by
Replying to SimonKing:
That's bad. Perhaps its a difference in new line / carriage return characters?
RuntimeError: [Errno 5] Input/output error
seems to indicate something more low-level related to the OS (for example, the handling of pseudo-tty's).
Probably I need to build sage on bsd.math before being able to test.
You can start working from the non-released sage-4.7.1.alpha0: http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar. This currently includes #7377 but not #10296, but this is of course subject to change.
comment:33 in reply to: ↑ 32 Changed 8 years ago by
Replying to jdemeyer:
You can start working from the non-released sage-4.7.1.alpha0: http://boxen.math.washington.edu/home/release/sage-4.7.1.alpha0/sage-4.7.1.alpha0.tar. This currently includes #7377 but not #10296, but this is of course subject to change.
Thank you, but the compilation of sage-4.7 on bsd.math is already in the final stages (building documentation, if I am not mistaken).
comment:34 in reply to: ↑ 32 Changed 8 years ago by
Replying to jdemeyer:
RuntimeError: [Errno 5] Input/output error
seems to indicate something more low-level related to the OS (for example, the handling of pseudo-tty's).
That could be. I already found: If you quit gap by sending "quit;" through the gap interface (by gap.eval) and then try to evaluate a command, then you get a different error or error message on bsd.math than on my computer.
What my patch did was to search for "EOL" in the error message, which should be fast and suffices on my computer (Linux). If it does not suffice (as on bsd.math) then one could still test gap._expect.isalive(). That's slower, but apparently more robust.
The other error you are reporting seems similar in spirit, but is in a different place.
comment:35 Changed 8 years ago by
I just submitted a new patch that solves part of the problem: There are different error types raised on linux and non-linux machines if a pexpect interfaces crashes. But there are some doctest failures left, so, it still needs work.
comment:36 Changed 8 years ago by
The problem with the remaining doctest failures is that my patch introduces a new argument first_call
to sage.interfaces.expect.Expect._eval_line
: The idea is to restart the interface if it is crashed unexpectedly and then re-eval the line, but only once, to avoid infinite loops. The first_call
argument tells whether it is the first evaluation, or the re-evaluation after a crash.
But some interfaces override _eval_line
and don't know about the new argument. It should thus be added.
comment:37 Changed 8 years ago by
Fortunately I can do some optional tests (e.g. maple) on bsd.math. It revealed another problem: Some code relies on a specific error message. But in my patch, that error is caught and replaced by a generic error telling that the interface crashed. That should be taken care of as well.
comment:38 Changed 8 years ago by
- Status changed from new to needs_review
I think I got it solved: The long doctests in sage/interfaces pass both on my machine and on bsd.math. This is based on sage-4.7.
In addition, the optional tests of sage/interfaces/maple.py pass on bsd.math as well, with the exception of those that fail with pure sage-4.7 anyway (see #11288).
Depends on #7377.
comment:39 Changed 8 years ago by
PS: Since the new patch is not totally trivial, I think someone should look at it, i.e., not simply restoring the old positive review.
comment:40 Changed 8 years ago by
*bump*
Any reviewers?
comment:41 Changed 8 years ago by
Actually from the description of the first patch it could help with some problems in #9958 so I will take a look and test them on linux and OS X.
comment:42 follow-up: ↓ 43 Changed 8 years ago by
- Status changed from needs_review to needs_work
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
sage -t -long "devel/sage/sage/interfaces/gap.py" ********************************************************************** File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514: sage: gap.interrupt(timeout=1) Expected: True Got: False **********************************************************************
This is a test introduced in trac10296_singular_overhead.patch
comment:43 in reply to: ↑ 42 ; follow-up: ↓ 44 Changed 8 years ago by
Replying to fbissey:
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
sage -t -long "devel/sage/sage/interfaces/gap.py" ********************************************************************** File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514: sage: gap.interrupt(timeout=1) Expected: True Got: False **********************************************************************
That test is really annoying. I tried to make it more reliable by adding the timeout parameter, but apparently it does not work like that.
However, the main point is that without the patch, we had
sage: cutoff = gap._eval_using_file_cutoff sage: gap._eval_using_file_cutoff = 4 sage: gap._eval_line('while(1=1) do i:=1;; od;', wait_for_prompt=False) <Runs forever>
In other words, we would not even come to the line gap.interrupt()
.
I would not mind if the new test simply made sure that no error is raised. It does not matter, IMHO, whether we have the return value True
or False
. So, perhaps change it into
sage: gap.interrupt(timeout=1) is not None True
or
sage: gap.interrupt(timeout=1) ...
?
comment:44 in reply to: ↑ 43 Changed 8 years ago by
Replying to SimonKing:
Replying to fbissey:
On OS X 10.5.8 with 4.7.1.alpha1 I get the following after applying the patch
sage -t -long "devel/sage/sage/interfaces/gap.py" ********************************************************************** File "/Users/frb15/Desktop/sage-4.7.1.alpha1/devel/sage/sage/interfaces/gap.py", line 514: sage: gap.interrupt(timeout=1) Expected: True Got: False **********************************************************************That test is really annoying. I tried to make it more reliable by adding the timeout parameter, but apparently it does not work like that.
However, the main point is that without the patch, we had
sage: cutoff = gap._eval_using_file_cutoff sage: gap._eval_using_file_cutoff = 4 sage: gap._eval_line('while(1=1) do i:=1;; od;', wait_for_prompt=False) <Runs forever>In other words, we would not even come to the line
gap.interrupt()
.I would not mind if the new test simply made sure that no error is raised. It does not matter, IMHO, whether we have the return value
True
orFalse
. So, perhaps change it intosage: gap.interrupt(timeout=1) is not None Trueor
sage: gap.interrupt(timeout=1) ...?
The later one would imply that the output is random, but we would miss it if an error was raised so I guess the former would be better.
gap.interrupt(timeout=1) is not None
has my vote.
comment:45 follow-up: ↓ 46 Changed 8 years ago by
- Cc leif added
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing testlong
rather than ptestlong
...
Can someone (finish? and) review this, or what's stopping it?
comment:46 in reply to: ↑ 45 ; follow-up: ↓ 47 Changed 8 years ago by
Replying to leif:
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing
testlong
rather thanptestlong
...Can someone (finish? and) review this, or what's stopping it?
I think the problem was that a couple of weeks ago I had other things on my mind, and thus I forgot to do the change into gap.interrupt(timeout=1) is not None
. I guess I'll do it now.
comment:47 in reply to: ↑ 46 Changed 8 years ago by
Replying to SimonKing:
Replying to leif:
The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing
testlong
rather thanptestlong
...I think the problem was that a couple of weeks ago I had other things on my mind, and thus I forgot to do the change into
gap.interrupt(timeout=1) is not None
. I guess I'll do it now.
That would be nice. Some tests (e.g. sandpile
) here take more than 20 minutes each when running make testlong
, which is totally odd.
comment:48 Changed 8 years ago by
Also, the first patch doesn't apply to sage-4.7.rc2. So, I'll take care of that as well.
Concerning idleness: It seems that there is a certain kernel patch for Ubuntu that improves the pexpect performance. Namely, the problem with some OS like Ubuntu is that the latency of pseudo-tty is to much (if I understood correctly). After our sysadmin applied that patch, the problem vanished.
So, the main problem with pexpect on some platforms is not a Sage problem. That said, reducing the traffic in the pexpect interface is a good idea anyway...
comment:49 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
I have updated the first patch, which should now apply against sage-4.7.1.rc2. I have changed the doctest in question, so that now it is only tested that interrupting gap has worked.
For the patchbot:
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch
Changed 8 years ago by
Modify garbage collection in Singular interface, which reduces the overhead. Synchronization of Gap interface.
comment:50 Changed 8 years ago by
I had to rebase the first patch against sage-4.7.1.rc1. I had a typo in my previous post: The old patch was relative sage-4.7.rc2, not 4.7.1.rc2.
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch
By the way: It still needs review...
comment:51 Changed 8 years ago by
Any reviewer for a patch that improves the performance of the Singular pexpect interface?
comment:52 Changed 8 years ago by
patchbot claims tests failed but the full log shows all tests passed. Not sure what happened there. I'm running ptestlong on sage.math now.
comment:53 follow-up: ↓ 54 Changed 8 years ago by
I re-read the patches and I still think they are good, so I'm close to a positive review. One - perhaps nitpicking - point though: shouldn't first_call
be restart_if_needed
or something? "First call" isn't very telling what it does?
comment:54 in reply to: ↑ 53 Changed 8 years ago by
Replying to malb:
I re-read the patches and I still think they are good, so I'm close to a positive review. One - perhaps nitpicking - point though: shouldn't
first_call
berestart_if_needed
or something? "First call" isn't very telling what it does?
That sounds like a good idea.
Changed 7 years ago by
comment:55 Changed 7 years ago by
I've added a patch which makes this change. Let's get this in.
comment:56 Changed 7 years ago by
- Description modified (diff)
Martin, can you look whether my patches together with Mike's improvement of the wording is fine?
Apply trac10296_singular_overhead.patch trac10296_expect_on_nonlinux.patch trac_10296-restart_if_needed.patch
comment:57 Changed 7 years ago by
Hi,
- trac_10296-restart_if_needed.patch has no commit message
- I get lots of errors when applying trac10296_expect_on_nonlinux.patch to 5.0-beta6?
comment:58 Changed 7 years ago by
- Status changed from needs_review to needs_work
- Work issues set to rebase
Bad. Needs work, then...
Concerning the missing commit message: I think I'll produce a combined patch with a commit message.
comment:59 Changed 7 years ago by
- Dependencies set to #11431
The first patch only applies with fuzz 2, because of #11431 (-> new dependency). The conflict is in the author list, easy to fix.
comment:60 Changed 7 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues rebase deleted
I produced a combined patch and verified that it cleanly applies on sage-5.0.beta6.
I started doctests without the patch, and later I will repeat with the patch. But for now it needs review...
Apply trac10296_singular_overhead_combined.patch
comment:61 Changed 7 years ago by
FWIW: The doc tests of sage-5.0.beta6 with the combined patch applied pass.
comment:62 follow-up: ↓ 63 Changed 7 years ago by
I only have some very minor comments:
- Could you escape EOFError with "
" backquotes so it's printed as code
- You wrote: "Since nesting of _eval_line can not occur during garbage collection" can you explain that, I am not sure I understand.
- why did you indent REMARK etc. further in pid()?
comment:63 in reply to: ↑ 62 Changed 7 years ago by
Replying to malb:
I only have some very minor comments:
- Could you escape EOFError with "
" backquotes so it's printed as code
Done
- You wrote: "Since nesting of _eval_line can not occur during garbage collection" can you explain that, I am not sure I understand.
I elaborated the comment.
Without my patch, garbage collection would result in a call to _eval_line. So, if garbage collection occurs while being in _eval_line, we would have two nested calls to _eval_line. The inner _eval_line would receive the Singular prompt and return. The outer _eval_line would also wait for a Singular prompt -- but Singular would not provide a second prompt. Result: A deadlock.
To my understanding, this is why synchronisation of the Singular interface was necessary. But with my patch, garbage collection does not involve an additional call to _eval_line, and thus the deadlock is prevented without synchronisation.
- why did you indent REMARK etc. further in pid()?
To the contrary, the first line of the doc string of pid() needed to be indented more! Fixed with the new patch version.
Apply trac10296_singular_overhead_combined.patch
Changed 7 years ago by
Modify garbage collection in Singular interface, which reduces the overhead. Synchronization of Gap interface. Cope with non-linux pexpect errors
comment:64 Changed 7 years ago by
Oops, I only had a single backtick around EOFError
-- hence, it would be typeset as LaTeX
. So, I have updated my patch, with double backticks around EOFError
.
comment:65 Changed 7 years ago by
- Status changed from needs_review to positive_review
I am satisfied.
But out of curiosity, do you have a simple before/after benchmark you could add to this *ticket*?
comment:66 Changed 7 years ago by
- Merged in set to sage-5.0.beta8
- Resolution set to fixed
- Status changed from positive_review to closed
comment:67 Changed 7 years ago by
There is a doctest error on OpenSolaris?, please have a look at #12687.
comment:68 follow-up: ↓ 69 Changed 7 years ago by
I have also seen the following error:
sage -t --long -force_lib devel/sage/sage/interfaces/expect.py ********************************************************************** File "/home/buildbot/build/sage/cleo-1/cleo_full/build/sage-5.0.beta9/devel/sage-main/sage/interfaces/expect.py", line 829: sage: singular.interrupt(timeout=1) Expected: False Got: True **********************************************************************
What does that mean? It is really a failure or should we just increase the timeout?
comment:69 in reply to: ↑ 68 ; follow-up: ↓ 70 Changed 7 years ago by
Replying to jdemeyer:
What does that mean? It is really a failure or should we just increase the timeout?
That error has always been mysterious to me.
Frankly, I need a break, in the sense of I need to focus on my main project (cohomology and ext algebras) and on my family. Martin, would you be able to take over?
comment:70 in reply to: ↑ 69 Changed 7 years ago by
Replying to SimonKing:
Replying to jdemeyer:
What does that mean? It is really a failure or should we just increase the timeout?
That error has always been mysterious to me.
Frankly, I need a break, in the sense of I need to focus on my main project (cohomology and ext algebras) and on my family.
Yes, have a "break". :-)
Since cleo is not the fastest machine, and I assume the above occurred when doctesting in parallel, I would either ignore it (if it doesn't happen too often), or try with increasing the timeout (to a few seconds) and if that helps, change the doctest permanently.
For the test I don't think it matters whether we wait 1 or 5 seconds, probably even more; it's only important that Singular finally does get interrupted (within perhaps at most half a minute, say).
Committing the kills in
singular.eval
by prepending stuff to the given command turned out to break a lot of doc tests, since they test against the given command showing up in the output. But I think one could be even closer to the Gap interface.Recall that in the Gap interface, variables are freed by allowing to re-use their names. So, memory allocated in Gap is only freed when the old variable is actually overwritten.
In Singular, memory is freed by the command
kill
. I suggest to use this command not insidesingular.eval
but insideSingularElement.__init__
: It is put in front of the command that creates the new variable. Then, the analogy between Gap and Singular interfaces is rather close: Memory in Gap or in Singular is freed only when a new Element is created -- by overwriting the old stuff in the case of Gap, or by killing the old stuff in the case of Singular.Moreover, I removed the synchronisation code, since it seems to me that synchronisation is not really necessary if the garbage collection is not using an additional _eval_line (which is the case with my suggestion).
So, there remains only one call to select.select when creating a new Singular element, rather than two plus the number of old variables to be killed.
Indeed, my approach reduces the overhead in using the Singular interface by 2/3 in the test above!
But there occur two doctest failures, that I have to deal with now.