Skip to content

Commit 7fd430b

Browse files
[GraphProxy] refine implementation of LazyDetails (#2766)
<!-- Thanks for your contribution! please review https://github.com/alibaba/GraphScope/blob/main/CONTRIBUTING.md before opening an issue. --> ## What do these changes do? <!-- Please give a short brief about these changes. --> Refine implementation of `LazyDetails` to avoid memory leak. Co-authored-by: Longbin Lai <[email protected]>
1 parent 4b24146 commit 7fd430b

File tree

3 files changed

+82
-297
lines changed

3 files changed

+82
-297
lines changed

interactive_engine/executor/ir/graph_proxy/src/adapters/csr_store/read_graph.rs

+33-106
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
1616
use std::convert::TryFrom;
1717
use std::fmt;
18-
use std::sync::atomic::{AtomicPtr, Ordering};
1918
use std::sync::Arc;
2019

2120
use ahash::HashMap;
@@ -255,24 +254,14 @@ struct LazyVertexDetails {
255254
// Specifically, Some(vec![]) indicates we need all properties
256255
// and None indicates we do not need any property
257256
prop_keys: Option<Vec<NameOrId>>,
258-
inner: AtomicPtr<LocalVertex<'static, DefaultId, DefaultId>>,
257+
inner: LocalVertex<'static, DefaultId, DefaultId>,
259258
}
260259

261260
impl_as_any!(LazyVertexDetails);
262261

263262
impl LazyVertexDetails {
264263
pub fn new(v: LocalVertex<'static, DefaultId, DefaultId>, prop_keys: Option<Vec<NameOrId>>) -> Self {
265-
let ptr = Box::into_raw(Box::new(v));
266-
LazyVertexDetails { prop_keys, inner: AtomicPtr::new(ptr) }
267-
}
268-
269-
fn get_vertex_ptr(&self) -> Option<*mut LocalVertex<'static, DefaultId, DefaultId>> {
270-
let ptr = self.inner.load(Ordering::SeqCst);
271-
if ptr.is_null() {
272-
None
273-
} else {
274-
Some(ptr)
275-
}
264+
LazyVertexDetails { prop_keys, inner: v }
276265
}
277266
}
278267

@@ -288,20 +277,14 @@ impl fmt::Debug for LazyVertexDetails {
288277
impl Details for LazyVertexDetails {
289278
fn get_property(&self, key: &NameOrId) -> Option<PropertyValue> {
290279
if let NameOrId::Str(key) = key {
291-
if let Some(ptr) = self.get_vertex_ptr() {
292-
unsafe {
293-
if key == "id" {
294-
let mask = (1_usize << LABEL_SHIFT_BITS) - 1;
295-
let original_id = ((*ptr).get_id() & mask) as i64;
296-
Some(PropertyValue::Owned(Object::Primitive(Primitives::Long(original_id))))
297-
} else {
298-
(*ptr)
299-
.get_property(key)
300-
.map(|prop| PropertyValue::Borrowed(to_borrow_object(prop)))
301-
}
302-
}
280+
if key == "id" {
281+
let mask = (1_usize << LABEL_SHIFT_BITS) - 1;
282+
let original_id = (self.inner.get_id() & mask) as i64;
283+
Some(PropertyValue::Owned(Object::Primitive(Primitives::Long(original_id))))
303284
} else {
304-
None
285+
self.inner
286+
.get_property(key)
287+
.map(|prop| PropertyValue::Borrowed(to_borrow_object(prop)))
305288
}
306289
} else {
307290
info!("Have not support getting property by prop_id in exp_store yet");
@@ -311,46 +294,26 @@ impl Details for LazyVertexDetails {
311294

312295
fn get_all_properties(&self) -> Option<HashMap<NameOrId, Object>> {
313296
// the case of get_all_properties from vertex;
314-
let props = if let Some(ptr) = self.get_vertex_ptr() {
315-
unsafe {
316-
if let Some(prop_key_vals) = (*ptr).get_all_properties() {
317-
let mut all_props: HashMap<NameOrId, Object> = prop_key_vals
318-
.into_iter()
319-
.map(|(prop_key, prop_val)| (prop_key.into(), to_object(prop_val)))
320-
.collect();
321-
let mask = (1_usize << LABEL_SHIFT_BITS) - 1;
322-
let original_id = ((*ptr).get_id() & mask) as i64;
323-
all_props.insert(
324-
NameOrId::Str("id".to_string()),
325-
Object::Primitive(Primitives::Long(original_id)),
326-
);
327-
Some(all_props)
328-
} else {
329-
None
330-
}
331-
}
297+
if let Some(prop_key_vals) = self.inner.get_all_properties() {
298+
let mut all_props: HashMap<NameOrId, Object> = prop_key_vals
299+
.into_iter()
300+
.map(|(prop_key, prop_val)| (prop_key.into(), to_object(prop_val)))
301+
.collect();
302+
let mask = (1_usize << LABEL_SHIFT_BITS) - 1;
303+
let original_id = (self.inner.get_id() & mask) as i64;
304+
all_props
305+
.insert(NameOrId::Str("id".to_string()), Object::Primitive(Primitives::Long(original_id)));
306+
Some(all_props)
332307
} else {
333308
None
334-
};
335-
props
309+
}
336310
}
337311

338312
fn get_property_keys(&self) -> Option<Vec<NameOrId>> {
339313
self.prop_keys.clone()
340314
}
341315
}
342316

343-
impl Drop for LazyVertexDetails {
344-
fn drop(&mut self) {
345-
let ptr = self.inner.load(Ordering::SeqCst);
346-
if !ptr.is_null() {
347-
unsafe {
348-
std::ptr::drop_in_place(ptr);
349-
}
350-
}
351-
}
352-
}
353-
354317
/// LazyEdgeDetails is used for local property fetching optimization.
355318
/// That is, the required properties will not be materialized until LazyEdgeDetails need to be shuffled.
356319
#[allow(dead_code)]
@@ -360,91 +323,55 @@ struct LazyEdgeDetails {
360323
// Specifically, Some(vec![]) indicates we need all properties
361324
// and None indicates we do not need any property,
362325
prop_keys: Option<Vec<NameOrId>>,
363-
inner: AtomicPtr<LocalEdge<'static, DefaultId, DefaultId>>,
326+
inner: LocalEdge<'static, DefaultId, DefaultId>,
364327
}
365328

366329
impl_as_any!(LazyEdgeDetails);
367330

368331
impl LazyEdgeDetails {
369332
pub fn new(e: LocalEdge<'static, DefaultId, DefaultId>, prop_keys: Option<Vec<NameOrId>>) -> Self {
370-
let ptr = Box::into_raw(Box::new(e));
371-
LazyEdgeDetails { prop_keys, inner: AtomicPtr::new(ptr) }
372-
}
373-
374-
fn get_edge_ptr(&self) -> Option<*mut LocalEdge<'static, DefaultId, DefaultId>> {
375-
let ptr = self.inner.load(Ordering::SeqCst);
376-
if ptr.is_null() {
377-
None
378-
} else {
379-
Some(ptr)
380-
}
333+
LazyEdgeDetails { prop_keys, inner: e }
381334
}
382335
}
383336

384337
impl fmt::Debug for LazyEdgeDetails {
385338
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
386339
f.debug_struct("LazyEdgeDetails")
387340
.field("prop_keys", &self.prop_keys)
388-
.field("inner", &self.inner)
341+
// .field("inner", &self.inner)
389342
.finish()
390343
}
391344
}
392345

393346
impl Details for LazyEdgeDetails {
394347
fn get_property(&self, key: &NameOrId) -> Option<PropertyValue> {
395348
if let NameOrId::Str(key) = key {
396-
let ptr = self.get_edge_ptr();
397-
if let Some(ptr) = ptr {
398-
unsafe {
399-
(*ptr)
400-
.get_property(key)
401-
.map(|prop| PropertyValue::Borrowed(to_borrow_object(prop)))
402-
}
403-
} else {
404-
None
405-
}
349+
self.inner
350+
.get_property(key)
351+
.map(|prop| PropertyValue::Borrowed(to_borrow_object(prop)))
406352
} else {
407353
info!("Have not support getting property by prop_id in experiments store yet");
408354
None
409355
}
410356
}
411357

412358
fn get_all_properties(&self) -> Option<HashMap<NameOrId, Object>> {
413-
// the case of get_all_properties from vertex;
414-
let props = if let Some(ptr) = self.get_edge_ptr() {
415-
unsafe {
416-
if let Some(prop_key_vals) = (*ptr).get_all_properties() {
417-
let all_props: HashMap<NameOrId, Object> = prop_key_vals
418-
.into_iter()
419-
.map(|(prop_key, prop_val)| (prop_key.into(), to_object(prop_val)))
420-
.collect();
421-
Some(all_props)
422-
} else {
423-
None
424-
}
425-
}
359+
if let Some(prop_key_vals) = self.inner.get_all_properties() {
360+
let all_props: HashMap<NameOrId, Object> = prop_key_vals
361+
.into_iter()
362+
.map(|(prop_key, prop_val)| (prop_key.into(), to_object(prop_val)))
363+
.collect();
364+
Some(all_props)
426365
} else {
427366
None
428-
};
429-
props
367+
}
430368
}
431369

432370
fn get_property_keys(&self) -> Option<Vec<NameOrId>> {
433371
self.prop_keys.clone()
434372
}
435373
}
436374

437-
impl Drop for LazyEdgeDetails {
438-
fn drop(&mut self) {
439-
let ptr = self.inner.load(Ordering::SeqCst);
440-
if !ptr.is_null() {
441-
unsafe {
442-
std::ptr::drop_in_place(ptr);
443-
}
444-
}
445-
}
446-
}
447-
448375
#[inline]
449376
fn to_object<'a>(ref_item: RefItem<'a>) -> Object {
450377
match ref_item {

0 commit comments

Comments
 (0)