Opened 3 years ago

Closed 3 years ago

#21720 closed enhancement (fixed)

Fix memory allocations in sparse_graph.pyx

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.5
Component: cython Keywords:
Cc: jmantysalo Merged in:
Authors: Jeroen Demeyer Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: 5d1b42e (Commits) Commit: 5d1b42ec9d101f45af7ea1fa77dab3d1784fa700
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

  1. Use checking allocation functions from cysignals to avoid a crash when running out of memory.
  1. A malloc(n) followed by a memset(..., 0, n) should be a calloc().

Change History (20)

comment:1 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/fix_some_memory_allocations_in_sparse_graph_pyx

comment:5 Changed 3 years ago by jdemeyer

  • Commit set to 5d1b42ec9d101f45af7ea1fa77dab3d1784fa700
  • Status changed from new to needs_review

New commits:

5d1b42eFix memory allocations

comment:6 Changed 3 years ago by jdemeyer

  • Summary changed from Fix some memory allocations in sparse_graph.pyx to Fix memory allocations in sparse_graph.pyx

comment:7 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:8 follow-up: Changed 3 years ago by jmantysalo

I don't know enough cython to review this, but at least it seems to work. With /proc/sys/vm/overcommit_memory as 0 or 1 it ends with MemoryError: failed to allocate 18446744072098938880 * 8 bytes, with 2 the error message is MemoryError: failed to allocate 1342177280 * 8 bytes.

comment:9 Changed 3 years ago by jmantysalo

Btw, is there same problem for example in src/sage/combinat/degree_sequences.pyx

    sig_on()
    seq = <unsigned char *> sig_malloc((n+1)*sizeof(unsigned char))
    memset(seq,0,(n+1)*sizeof(unsigned char))
    sig_off()

?

comment:10 follow-up: Changed 3 years ago by jdemeyer

I can't fix all problems in Sage...

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by jmantysalo

Replying to jdemeyer:

I can't fix all problems in Sage...

I know... But will it help someone if I open a ticket and search other uses of unnecessay memset?

comment:12 in reply to: ↑ 11 Changed 3 years ago by jdemeyer

Replying to jmantysalo:

Replying to jdemeyer:

I can't fix all problems in Sage...

I know... But will it help someone if I open a ticket and search other uses of unnecessay memset?

To be honest... unless you plan to fix it yourself, there is probably not much point.

comment:13 in reply to: ↑ 8 ; follow-ups: Changed 3 years ago by jdemeyer

Replying to jmantysalo:

With /proc/sys/vm/overcommit_memory as 0 or 1 it ends with MemoryError: failed to allocate 18446744072098938880 * 8 bytes

What is "it" in the sentence above?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by jmantysalo

Replying to jdemeyer:

Replying to jmantysalo:

With /proc/sys/vm/overcommit_memory as 0 or 1 it ends with MemoryError: failed to allocate 18446744072098938880 * 8 bytes

What is "it" in the sentence above?

Error message.

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

Replying to jmantysalo:

Replying to jdemeyer:

Replying to jmantysalo:

With /proc/sys/vm/overcommit_memory as 0 or 1 it ends with MemoryError: failed to allocate 18446744072098938880 * 8 bytes

What is "it" in the sentence above?

Error message.

The error message when doing what?

comment:16 Changed 3 years ago by jmantysalo

Uh, sorry. I tested Graph(10^10), but wrote that only in sage-devel, not here.

comment:17 in reply to: ↑ 13 Changed 3 years ago by jdemeyer

Replying to jdemeyer:

Replying to jmantysalo:

With /proc/sys/vm/overcommit_memory as 0 or 1 it ends with MemoryError: failed to allocate 18446744072098938880 * 8 bytes

By the way, this number shows that some integer overflow is happening also (a different issue from this ticket).

The number 18446744072098938880 equals <size_t><int>(10 << 28) where the correct value would be just <size_t>(10 << 28) = 2684354560.

comment:18 Changed 3 years ago by jdemeyer

Please review...

comment:19 Changed 3 years ago by mmezzarobba

  • Reviewers set to Marc Mezzarobba
  • Status changed from needs_review to positive_review

comment:20 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_some_memory_allocations_in_sparse_graph_pyx to 5d1b42ec9d101f45af7ea1fa77dab3d1784fa700
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.