Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#10296 closed enhancement (fixed)

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

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

Description (last modified by jdemeyer)

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.

Change History (23)

comment:1 Changed 8 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 8 years ago by AlexanderDreyer

  • Cc AlexanderDreyer added

comment:3 Changed 8 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 8 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 8 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 8 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 8 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 8 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 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)
Note: See TracTickets for help on using tickets.