Opened 9 years ago

Last modified 7 years ago

#10296 closed enhancement

Singular interface wasting time by waiting for the prompt too often — at Version 60

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:
Authors: Simon King Reviewers: Martin Albrecht
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11431 Stopgaps:

Description (last modified by SimonKing)

There are two fundamental differences between the Singular and the Gap interfaces:

  1. The Singular interface uses a synchronisation method in each call to its eval method. Gap doesn't.
  2. 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:

  1. singular.eval(cmd) is currently simply passing cmd to _eval_line, after synchronisation and after killing unused variables. I suggest to add the code for killing unused variables to cmd, thus using _eval_line only once.
  2. 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

Change History (63)

comment:1 Changed 9 years ago by SimonKing

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 inside singular.eval but inside SingularElement.__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.

comment:2 Changed 9 years ago by AlexanderDreyer

  • Cc AlexanderDreyer added

comment:3 Changed 9 years ago by SimonKing

  • Authors set to Simon King
  • 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 9 years ago by SimonKing

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: Changed 9 years ago by SimonKing

  • 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 9 years ago by SimonKing

  • 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 9 years ago by SimonKing

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 9 years ago by SimonKing

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 SimonKing

  • 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 SimonKing

  • 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 SimonKing

  • 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 malb

  • 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: Changed 8 years ago by jdemeyer

  • 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: Changed 8 years ago by 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.

comment:15 in reply to: ↑ 14 Changed 8 years ago by jdemeyer

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: Changed 8 years ago by SimonKing

  • 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: Changed 8 years ago by SimonKing

  • 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 SimonKing

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 malb

  • Status changed from needs_review to positive_review

yes, back to positive review.

comment:20 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by wjp

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: Changed 8 years ago by SimonKing

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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:24 in reply to: ↑ 21 Changed 8 years ago by jdemeyer

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: Changed 8 years ago by 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?

comment:26 in reply to: ↑ 25 Changed 8 years ago by jdemeyer

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: Changed 8 years ago by 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.

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 SimonKing

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 jpflori

Ok, I've now done 500 tests of both without problems.

comment:30 Changed 8 years ago by jdemeyer

  • 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: Changed 8 years ago by SimonKing

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: Changed 8 years ago by jdemeyer

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 SimonKing

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 SimonKing

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 SimonKing

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 SimonKing

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 SimonKing

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.

Changed 8 years ago by SimonKing

Make the automatic restart of pexpect interfaces work on non-linux machines

comment:38 Changed 8 years ago by SimonKing

  • 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 SimonKing

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 jdemeyer

*bump*

Any reviewers?

comment:41 Changed 8 years ago by fbissey

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: Changed 8 years ago by fbissey

  • 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: Changed 8 years ago by 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 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 fbissey

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 or False. So, perhaps change it into

sage: gap.interrupt(timeout=1) is not None
True

or

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: Changed 8 years ago by leif

  • 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: Changed 8 years ago by SimonKing

Replying to leif:

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?

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 leif

Replying to SimonKing:

Replying to leif:

The "idling" doctests (on e.g. Ubuntu) are really annoying, especially when doing testlong rather than ptestlong...

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 SimonKing

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 SimonKing

  • 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 SimonKing

Modify garbage collection in Singular interface, which reduces the overhead. Synchronization of Gap interface.

comment:50 Changed 8 years ago by SimonKing

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 SimonKing

Any reviewer for a patch that improves the performance of the Singular pexpect interface?

comment:52 Changed 8 years ago by malb

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: Changed 8 years ago by 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 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 SimonKing

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 be restart_if_needed or something? "First call" isn't very telling what it does?

That sounds like a good idea.

Changed 8 years ago by mhansen

comment:55 Changed 8 years ago by mhansen

I've added a patch which makes this change. Let's get this in.

comment:56 Changed 7 years ago by SimonKing

  • 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 malb

Hi,

comment:58 Changed 7 years ago by SimonKing

  • 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 SimonKing

  • 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 SimonKing

  • 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

Note: See TracTickets for help on using tickets.