WIP: Bug: Split out MemIO from IOStream #1244

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Member

kmsquire commented Sep 1, 2012

Discussion reference: https://groups.google.com/d/topic/julia-dev/4bDcs9A5614/discussion

Something like this is what I think a MemIO type could look like.

However, this patch is horribly BROKEN: when building sys.ji, julia suddenly decides she wants more memory than my machine has, and everything freezes until the OOM killer kicks in. From my changes, it is not at all obvious to me why. Help!

Cheers, Kevin

(I'll likely be unresponsive for the rest of the weekend... Happy Labor Day!)

Owner

JeffBezanson commented Sep 1, 2012

I see ios(s::MemIO) = ios(s), which might be causing a stack overflow.

Member

kmsquire commented Sep 4, 2012

Fixed this, but it still has the same issue. The issue actually occurred before I included ios(s) = s.ios. Then, s.ios was accessed directly anywhere that needed it (as in the original code), but I felt it was sketchy to depend on a field being present for an abstract type. So I added ios(s) for both MemIO and IOStream, and used ios(s) to access. Either way, that (and your fix) didn't make the problem go away.

Could it be that the ios(s) method is being inlined, and my "fix" above is simply disappearing? What would happen if a method tries to directly access a member of an abstract type?

(I do realize you don't intend to merge this, just trying to understand why what I thought was an innocuous change doesn't work.)

Owner

JeffBezanson commented Sep 4, 2012

Even if we don't merge this you may have found an interesting bug, which is quite worthwhile! I should take a closer look.
Accessing a member of an abstract type does not occur because abstract types don't have instances; you can only look up fields in actual values. It's fine if the concrete type is not known at compile time; there will still be an actual value there at run time.

Member

kmsquire commented Sep 5, 2012

I've rebased on the current master and reverted back to a more minimal state. The patch now just creates an IOS superclass for IOStream and MemIO, and changes the signature of most IOStream functions to take an IOS, except for the takebuf_*() functions, which take a MemIO.

Member

kmsquire commented Sep 5, 2012

And it still has the same issue, of course.

Owner

Keno commented Jan 5, 2013

I suppose this is superseded by the recent work on Buffers/IOStrings etc. @vtjnash Do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment