fplll / fpylll

A Python interface for https://github.com/fplll/fplll
GNU General Public License v2.0
119 stars 60 forks source link

Tracing = Memory leak #217

Closed lducas closed 2 years ago

lducas commented 2 years ago

When running big BKZ instances, recursive calls make BKZ trace huge, For example, 40 instances in parallel, I manage to choke a server that had 1.5TB of RAM... And I didn't even needed the trace in the first place, but there is no way around it in the current API.

The proposed patches:

malb commented 2 years ago

I see the problem, not sure about the solution.

  1. I'd rather have something like tracer=dummy_tracer or tracer=False for consistency with other methods
  2. Isn't the "right fix" to force a max depth for the recursion level of the tracer?

If you agree with 2, I can get on that.

lducas commented 2 years ago
  1. Sure. This means that the caller has to create the tracer, but we could also hook a special value tracer=True that create the tracer internally ?

  2. I don't get why tracing should be the default. But indeed a default max_depth might be also a good idea.

-- L

Le mer. 15 déc. 2021 à 10:48, Martin R. Albrecht @.***> a écrit :

I see the problem, not sure about the solution.

  1. I'd rather have something like tracer=dummy_tracer or tracer=False for consistency with other methods
  2. Isn't the "right fix" to force a max depth for the recursion level of the tracer?

If you agree with 2, I can get on that.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fplll/fpylll/pull/217#issuecomment-994586811, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQGTYHC3UYTL5XOJV75JR3URBP5RANCNFSM5KDEWNHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

malb commented 2 years ago

Yeah, we can make False an alias for dummy_tracer and True for a real one. We can also default to False. But sometimes you want to trace and then max_depth is useful :)

lducas commented 2 years ago

I am not opposing max_depth.

Though I wouldn't be sure how to instantiate it. There are X_0 many intermediate calls to the first svp, and X_1 many intermediate calls between reccursive cals to SVP, for value X_0 and X_1 I wouldn't dare to guess :D

So, I do 1, and then you do 2 ?

Le mer. 15 déc. 2021 à 10:56, Martin R. Albrecht @.***> a écrit :

Yeah, we can make False an alias for dummy_tracer and True for a real one. We can also default to False. But sometimes you want to trace and then max_depth is useful :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fplll/fpylll/pull/217#issuecomment-994598453, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQGTYF6QAV27LXIPHITI6DURBQ3XANCNFSM5KDEWNHQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

malb commented 2 years ago

Will do, hopefully this weekend.

malb commented 2 years ago

Back to you, how do you like my proposal?

lducas commented 2 years ago

Looks neat and tidy.